-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: add instrumentation scope support #336
Conversation
It also has attributes https://github.com/open-telemetry/opentelemetry-proto/blob/bd7cf55b6d45f3c587d2131b68a7e5a501bdb10c/opentelemetry/proto/common/v1/common.proto#L69-L81; Do we not want to support them for now? |
I was planning to just introduce the name first, but I guess it's better to introduce the version as well. Let me make the changes. For instrumentation scope attribute do you think it's better to make changes now ? |
I suggest adding the attributes as well so you don't have to come back to it again in future if somebody asks for it. |
hmm makes sense, regarding the implementation of it do see any cons with storing the attributes in a similar way to that of resource attributes. Here will we will name it as instrumentation_scope_string_key, instrumentation_scope_string_value. |
I don't see any issue. |
have added support for name, version and attributes. |
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.
LGTM 👍
|
||
scopeAttributes := attributesToSlice(scope.Attributes(), true) | ||
|
||
err := addAttrsToTagStatement(tagStatement, "instrumentation_scope", resources) |
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 should be using scopeAttributes
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.
Have updated it to scope
#344
Otel collector changes for SigNoz/signoz#2705