Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add service.name resource attribute to the collector's own telemetry #6152

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Sep 26, 2022

Resolves #6136

  • The default service.name is set to BuildInfo.Command (equals to "otelcol" for this repo).
  • The service.name can be overridden by the end user via telemetry config setting.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/add-service-name branch from 7ddfc9f to 2927551 Compare September 26, 2022 20:55
@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Sep 26, 2022

@jpkrohling looks like builder is failing because it imports a specific version of the Collector instead of the latest code in local directory (and the previous version doesn't contain the newly added BuildInfo.ServiceName field). Should we change builder to import latest Collecor or there is some other thinking behind this that I am not aware?

[UPDATE]: nevermind. Discussed with Bogdan and eliminated this change from the PR. All is good.

@tigrannajaryan tigrannajaryan marked this pull request as ready for review September 26, 2022 22:08
@tigrannajaryan tigrannajaryan requested review from a team and mx-psi September 26, 2022 22:08
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/add-service-name branch 6 times, most recently from 6c92422 to 3cd1854 Compare September 27, 2022 01:35
@codecov
Copy link

codecov bot commented Sep 27, 2022

Codecov Report

Base: 91.87% // Head: 91.77% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (342e359) compared to base (711b882).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 342e359 differs from pull request most recent head 6373d16. Consider uploading reports for the commit 6373d16 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6152      +/-   ##
==========================================
- Coverage   91.87%   91.77%   -0.11%     
==========================================
  Files         219      217       -2     
  Lines       13533    13359     -174     
==========================================
- Hits        12434    12260     -174     
  Misses        870      870              
  Partials      229      229              
Impacted Files Coverage Δ
service/telemetry.go 90.62% <100.00%> (+0.55%) ⬆️
confmap/confmap.go 90.99% <0.00%> (-2.21%) ⬇️
pdata/ptrace/traces.go 80.64% <0.00%> (-1.18%) ⬇️
pdata/pmetric/metrics.go 75.00% <0.00%> (-0.46%) ⬇️
pdata/plog/logs.go 96.29% <0.00%> (-0.26%) ⬇️
config/identifiable.go 100.00% <0.00%> (ø)
pdata/pcommon/common.go 89.92% <0.00%> (ø)
pdata/plog/plogotlp/logs.go 51.06% <0.00%> (ø)
pdata/ptrace/ptraceotlp/traces.go 51.06% <0.00%> (ø)
pdata/pmetric/pmetricotlp/metrics.go 55.81% <0.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

service/telemetry.go Show resolved Hide resolved
service/telemetry.go Outdated Show resolved Hide resolved
service/telemetry.go Outdated Show resolved Hide resolved
service/telemetry.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Sep 27, 2022

I think we have 3 open questions:

  • Should different distributions use different default value of service.name in the own telemetry?
  • Should Core and Contrib builds use different service.name?
  • Should we aim for short, more readable service.name like otelcol or for longer but more unambiguous, e.g. FQDN io.opentelemetry.collector?

Seems like we have different opinions here. Perhaps other @open-telemetry/collector-approvers and @open-telemetry/collector-contrib-approvers can chime in and suggest some arguments that help us answer these questions (i.e. provide some motivation why you think so)?

@bogdandrutu
Copy link
Member

Should different distributions use different service.name in the own telemetry?

Who do you ask for? Every distro can do what they want. If a distro owner comes and ask me, I would suggest a different service name since is a different service (even though has lots of common things with the core service).

Should Core and Contrib builds use different service.name?

I think so.

Should we aim for short, more readable service.name like otelcol or for longer but more unambiguous, e.g. FQDN io.opentelemetry.collector?

I've never seen a long name like io.opentelemetry.collector. I personally prefer up to 10 characters names, ideal 5-7.

@djaglowski
Copy link
Member

The specification says of service.name: MUST be the same for all instances of horizontally scaled services.

It also describes related fields, including service.namespace: A string value having a meaning that helps to distinguish a group of services, for example the team name that owns a group of services.

My interpretation of this is that service.name should be quite generic and service.namespace can optionally be used by distros and custom builds to provide additional specificity.

Applying this to Tigran's questions, I think service.name should be constant (eg otelcol), but we should support in some way that distros and custom builds can specify service.namespace in order to differentiate themselves.

@bogdandrutu
Copy link
Member

Applying this to Tigran's questions, I think service.name should be constant (eg otelcol), but we should support in some way that distros and custom builds can specify service.namespace in order to differentiate themselves.

Why? I think the configuration in the end should belong to the final user and not to the distributor, since this is a service deployed/monitored by that user not by the distributor. If the distributor also deploys this for the final user they control the config so they can do whatever they want.

@djaglowski
Copy link
Member

I think the configuration in the end should belong to the final user and not to the distributor, since this is a service deployed/monitored by that user not by the distributor. If the distributor also deploys this for the final user they control the config so they can do whatever they want.

Ok, fair point.

I guess I'm confused about whether the service namespace should uniquely identify the binary, or if it is purely a concern of the final user to describe it sufficiently for their needs.

@tigrannajaryan
Copy link
Member Author

we should support in some way that distros and custom builds can specify service.namespace in order to differentiate themselves.

We do support this already in the end user config. You can specify service.namespace (and any other attribute) in service.telemetry.resource config setting.

However, service.namespace is typically intended for grouping of team-wide services (e.g. all services that belong to Finance department), not for grouping of services by the distro. Although I can accept that such grouping may be a valid choice for some users I don't think we need to support it in any special way by encouraging distro owners to set their own service.namespace. I believe it is one of the attributes that truly belongs to the end users.

@tigrannajaryan
Copy link
Member Author

Should different distributions use different service.name in the own telemetry?

Who do you ask for? Every distro can do what they want. If a distro owner comes and ask me, I would suggest a different service name since is a different service (even though has lots of common things with the core service).

I am asking on behalf of the end user. What is the best from their perspective? If they happen to deploy Collectors from different distros do they want them to appear as the same service or as different services in their backend?

@TylerHelmuth
Copy link
Member

My two cents:

Should different distributions use different service.name in the own telemetry?

Ultimately the user should have the ability to set whatever service.name they want and we should provide a default value. I'd go as far as to say that users should be able to set any attributes they want on the collector's own telemetry. They are the ones that are going to use the telemetry to monitor their collectors, so we should let them do what they want with their data.

Should Core and Contrib builds use different service.name?

Sure, but I don't have strong feelings on this at all.

Should we aim for short, more readable service.name like otelcol or for longer but more unambiguous, e.g. FQDN io.opentelemetry.collector?

I'd vote shorter name as the default.

@tigrannajaryan
Copy link
Member Author

Ultimately the user should have the ability to set whatever service.name they want and we should provide a default value. I'd go as far as to say that users should be able to set any attributes they want on the collector's own telemetry. They are the ones that are going to use the telemetry to monitor their collectors, so we should let them do what they want with their data.

Yes, that's exactly what we already do in this PR. We allow the user to override service.name and set any other attributes they want. The question is precisely about what the default value of service.name should be. (I updated my earlier comment to make it clear).

@TylerHelmuth
Copy link
Member

Yes, that's exactly what we already do in this PR. We allow the user to override service.name and set any other attributes they want.

Yup, I'm agreeing with this PR's implementation.

As for actual default value, I'll put a vote in for BuildInfo.Command so we can gracefully achieve otelcol and otelcol-contrib without changing our build/release process.

@tigrannajaryan
Copy link
Member Author

I see a lot of support for using BuildInfo.Command as the default value for service.name. Anyone thinks this is a bad idea?

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/add-service-name branch from 342e359 to b6b5b2c Compare October 3, 2022 13:36
Resolves open-telemetry#6136

- The default service.name is set to BuildInfo.Command (equals to "otelcol" for this repo).
- The service.name can be overridden by the end user via telemetry config setting.
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/add-service-name branch from b6b5b2c to 6373d16 Compare October 3, 2022 13:39
@tigrannajaryan
Copy link
Member Author

@open-telemetry/collector-approvers I changed the default for service.name to BuildInfo.Command, PTAL.

@bogdandrutu bogdandrutu merged commit 3ecce47 into open-telemetry:main Oct 3, 2022
@tigrannajaryan tigrannajaryan deleted the feature/tigran/add-service-name branch October 3, 2022 21:07
@diego-lambrechts-wp
Copy link

Hi!. Can anyone please tell me in which section of the configuration the service.name can be set? Unfortunately I had no luck finding it in the documentation. Thank you!

@tigrannajaryan
Copy link
Member Author

Hi!. Can anyone please tell me in which section of the configuration the service.name can be set? Unfortunately I had no luck finding it in the documentation. Thank you!

As far as I remember:

service:
  telemetry:
    resource:
      service.name: <any string here>

@sjparkinson
Copy link

sjparkinson commented Jan 30, 2024

I was surprised to find that setting OTEL_SERVICE_NAME or OTEL_RESOURCE_ATTRIBUTES did not work. And that there was no mention of using the telemetry section for setting resource attributes at https://opentelemetry.io/docs/collector/configuration/#telemetry.

@TylerHelmuth
Copy link
Member

@sjparkinson that is because the Collector doesn't use the Go OTel SDK by default yet (but we're almost there). If you enable the featuregate to use the Otel Go exporter then those env vars should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add service.name resource attribute to the collector's own telemetry
9 participants