Skip to content

Conversation

@dpaasman00
Copy link
Contributor

Description

Adds a new field to component.BuildInfo to facilitate support for reporting the service.namespace semantic convention in OpAMP. Includes corresponding updates to resource attributes reported by the collector. Also adds a new parameter to the builder configuration to facilitate setting this via the OCB.

Link to tracking issue

Fixes #12505

Need follow up PR to OpAMP extension making use of this value.

Testing

Relevant unit tests updated.

Running the updated otelcorecol with logging enabled in service.telemetry and the new resource attribute for service.namespace shows up. This was done sending to another collector running the otlp receiver and logging via the debug exporter.

02-26-25_monitoring-output.txt

Documentation

@dpaasman00 dpaasman00 requested a review from a team as a code owner February 26, 2025 20:21
@dpaasman00 dpaasman00 requested a review from atoulme February 26, 2025 20:21
@dpaasman00 dpaasman00 force-pushed the dakotapaasman/bpop-2116-configurable-servicenamespace-value-in-ocb branch from 10a0443 to 832d433 Compare February 26, 2025 20:28
@codecov
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.20%. Comparing base (8cf42f3) to head (146215c).
Report is 126 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12508      +/-   ##
==========================================
+ Coverage   92.18%   92.20%   +0.02%     
==========================================
  Files         469      469              
  Lines       25390    25395       +5     
==========================================
+ Hits        23405    23416      +11     
+ Misses       1574     1568       -6     
  Partials      411      411              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dpaasman00 dpaasman00 changed the title add namespace parameter to component.BuildInfo [component] Add namespace parameter to component.BuildInfo Feb 27, 2025
@dpaasman00 dpaasman00 force-pushed the dakotapaasman/bpop-2116-configurable-servicenamespace-value-in-ocb branch from a0aced7 to 0e8b029 Compare February 27, 2025 18:09
@dpaasman00 dpaasman00 force-pushed the dakotapaasman/bpop-2116-configurable-servicenamespace-value-in-ocb branch from 742e731 to d5a3432 Compare March 3, 2025 12:11
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me.

@dpaasman00 dpaasman00 force-pushed the dakotapaasman/bpop-2116-configurable-servicenamespace-value-in-ocb branch from 9e2fe4e to b3427b6 Compare March 5, 2025 18:31
@dpaasman00 dpaasman00 requested a review from evan-bradley March 5, 2025 18:31
evan-bradley
evan-bradley previously approved these changes Mar 5, 2025
@evan-bradley evan-bradley added the ready-to-merge Code review completed; ready to merge by maintainers label Mar 5, 2025
@dpaasman00 dpaasman00 force-pushed the dakotapaasman/bpop-2116-configurable-servicenamespace-value-in-ocb branch from b3427b6 to 03bb387 Compare March 6, 2025 13:50
@dpaasman00 dpaasman00 force-pushed the dakotapaasman/bpop-2116-configurable-servicenamespace-value-in-ocb branch from 03bb387 to 146215c Compare March 7, 2025 13:56
@dpaasman00
Copy link
Contributor Author

Contrib tests failing

@bogdandrutu
Copy link
Member

I am not sure I understand, how this. Why do you not configure the "service.namespace" in the service::telemetry::resource?

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The service.namespace is a runtime identification configuration and not a build time identification configuration, so I am confused why we need a change to the build info.

@evan-bradley evan-bradley dismissed their stale review March 7, 2025 17:29

Need more discussion

@evan-bradley
Copy link
Contributor

@dpaasman00 I've discussed this offline with other core leads and received some feedback that we may need to make some changes. Could you make the following updates to this PR?

  1. Remove Namespace from component.BuildInfo. component is a stable module, so we need to be absolutely sure of anything we put in there. We will likely need to pass information from the builder down to the service through a new parameter for now, and we can consider moving it to component.BuildInfo in the future. Additionally, is there a term other than namespace that we could use to uniquely identify distributions in a way that doesn't include the binary name?
  2. Clarify whether service.namespace is the correct attribute for something set at build time. My initial impression was the same as Bogdan's, that service.namespace is primarily intended to describe the runtime context of a service than describe its functionality, but reading the docs and seeing how we set service.name, I'm not so clear on this. Could you determine whether we should set this information in service.namespace, or whether service.name may be more appropriate? If we don't set service.namespace, maybe we add a distro field and make the service.name attribute something like opentelemetry.otelcol.

@evan-bradley
Copy link
Contributor

A relatively minor aside: I read through the docs again and saw this:

Note: service.namespace and service.name are not intended to be concatenated for the purpose of forming a single globally unique name for the service.

I don't think this strictly impacts anything here since we can define the semantics if we concatenate the distro and binary inside service.name, but we do need to think about what it means if we put the distro into service.namespace.

@dpaasman00
Copy link
Contributor Author

After spending some time thinking about this, I agree that service.namespace is not the correct attribute for what we're trying to accomplish. The goal is to better differentiate collectors now that the OCB enables users to create and manage their own distributions and versions of the collector. To achieve this, I think we should add 2 new attributes to the service resource semantic convention, service.description and service.distribution.

  • service.description would be a long, human friendly description of what the collector is intended for. There is already a distribution.description config parameter for the builder that gets added to the BuildInfo, this would just need to get reported by the OpAMP extension once the semantic convention is established.
  • service.distribution would represent who created and manages the manifest the collector was built with. Implementing this attribute would require adding a new parameter to the builder, adding a new field to BuildInfo, and updating the OpAMP extension to report this value once the semantic convention is established.

I understand this approach would take a while since we're changing the semantic conventions and BuildInfo, but I do think it is a better approach to the original problem this PR was trying to solve.

@evan-bradley @bogdandrutu What do you think of this approach?

@evan-bradley
Copy link
Contributor

Thanks for your thoughts, I'm largely in agreement with everything you wrote. There is an existing proposal for something similar, but under the agent attribute namespace: open-telemetry/semantic-conventions#950. I think it would be worth checking on that PR or at the next OpAMP SIG meeting how we should proceed there.

@evan-bradley evan-bradley removed the ready-to-merge Code review completed; ready to merge by maintainers label Mar 11, 2025
@dpaasman00 dpaasman00 marked this pull request as draft March 17, 2025 15:13
@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 1, 2025
@dpaasman00 dpaasman00 closed this Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[component] Add service.namespace semantic convention to component.BuildInfo

5 participants