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 resource + attributes #6

Merged
merged 4 commits into from
May 2, 2023

Conversation

codeboten
Copy link
Contributor

Currently the only attribute that's required is service.name.

Currently the only attribute that's required is `service.name`.
@codeboten codeboten requested a review from a team April 26, 2023 22:44
@tsloughter
Copy link
Member

I don't think service.name should be required. It isn't required to set OTEL_SERVICE_NAME.

@codeboten
Copy link
Contributor Author

I don't think service.name should be required. It isn't required to set OTEL_SERVICE_NAME.

Fair, i figured since SDKs override it anyways, it would be handy to have it required, but i can remove it as well

@pirgeo
Copy link
Member

pirgeo commented Apr 27, 2023

Is there a way of setting it as recommended? Also: would the file-based config always be the "lowest" priority and overwritten when the value is set somewhere else?

@tsloughter
Copy link
Member

SDK's override it?

My initial thought when seeing this was an issue with Erlang because it is able to automatically set it for the user, but if the user sets it in the config this would be taken over the detected service name.

@pirgeo
Copy link
Member

pirgeo commented Apr 27, 2023

SDK's override it?

I don't think they should do that. I am trying to understand what the priority is.

My understanding is:

  • Use the service name provided in code (EDIT: on SDK setup, that is what I meant above)
  • If that isn't set, use the service name provided in the config file
  • If that isn't set, use the OTEL_SERVICE_NAME env var
  • If that isn't set, use the value provided by a resource detector
  • If none of these are set, use unknown_service

Am I missing something? Is the order okay?

@jack-berg
Copy link
Member

@pirgeo in the OTEP we discussed whether file configuration should include layering with other sources of config like environment variables, and decided no. See this comment. Working on codifying this in the specification with this line:

The SDK components MUST strictly reflect the Configuration and ignore the environment variable configuration scheme.

kitchen-sink-example.yaml Outdated Show resolved Hide resolved
schema/attributes.json Outdated Show resolved Hide resolved
@codeboten
Copy link
Contributor Author

codeboten commented Apr 27, 2023

SDK's override it?

@tsloughter I mispoke, I meant that the SDKs will default to unknown_service if no service name is set.

@pirgeo
Copy link
Member

pirgeo commented Apr 27, 2023

@jack-berg Thanks for the clarification. It seems I need to read the OTEP / discussion around it a bit more carefully! 😄

@codeboten
Copy link
Contributor Author

Moved the discussion around requiring service.name into its own issue, PTAL

Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Pending further discussion of service.name in #11 - this looks good to me.

@codeboten codeboten merged commit 280d924 into open-telemetry:main May 2, 2023
@codeboten codeboten deleted the codeboten/add-resource branch May 2, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants