-
Notifications
You must be signed in to change notification settings - Fork 216
feat: adding configurable deployment.name in otel #2115
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
base: main
Are you sure you want to change the base?
Conversation
|
I'm being conservative not to add an attribute if the operator does not explicitly set the config. It is up for discussion whether we want to use a default value or keep the conservative approach |
svix-jbrown
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem unreasonable, but a couple of questions to think about...
Question 1: I see that signoz recommends deployment.environment, but the official opentelemetry docs recommend deployment.environment.name; do you have any ideas as to which key is more commonly-used? Does signoz also support the "official" one?
Question 2: Do we want to special case deployment.environment, or add something like opentelemetry_resource_attributes: HashMap<String, String> to let people set other tags? Most otel applications just read from $OTEL_RESOURCE_ATTRIBUTES (although ours does not), which would be another option.
I would argue that we leave this fairly well defined for now, ie, don't allow attributes that we haven't explicitly set through configuration parameters. |
|
@svix-james searching through signoz source code, I dont think they support Anw should we proceed with this PR |
svix-james
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, apologies, I didn't read @svix-jbrown's previous message closely. We should definitely not support a non-standard resource name here.
|
Honestly at this point I would just argue for using the original proposal (#2110), i.e., passing by environment variable, and being done with it. The reason we wanted to avoid this is to prevent inadvertently sharing OTEL parameters between Svix and user-specific OTEL streams downstream, but I think we can avoid that if we're careful. @vanhtuan0409 Have you confirmed that setting the OTEL env var actually works for your non-standard attribute name? If so, please update this PR (no need to close/open another one) with the changes from #2110, and we can get this in. |
Motivation
Adding configuration to tag otel trace and metrics with
deployment.environmentattribute. Supporting signoz environmenthttps://signoz.io/docs/faqs/general/#q-how-to-setup-signoz-across-different-environments
Solution
As above