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

Upgrade OTEL version to 1.2 #5359

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kkondaka
Copy link
Collaborator

Description

Update OTEL version to 1.2

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • [X ] New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • [X ] Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Krishna Kondaka <[email protected]>
@KarstenSchnitter
Copy link
Collaborator

This changeset is also part of #5322. @kkondaka what is your opinion on backwards compatibility to OTel data containing instrumentation library fields?

Copy link
Collaborator

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

Thanks for providing that change. I would like to see some integration test using the old protobuf data format. Also, I have questions on the handling of instrumentation scope attributes. See the comments inline for the latter.

@@ -92,6 +89,7 @@ public class OTelProtoCodec {
static final String INSTRUMENTATION_LIBRARY_VERSION = "instrumentationLibrary.version";
static final String STATUS_CODE = "status.code";
static final String STATUS_MESSAGE = "status.message";
static final String ATTRIBUTES_KEY = "attributes";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't there be something like instrumentationScope.attributes? These attributes should be handled similar to resource, signal, or event attributes.

@@ -1202,6 +1103,9 @@ public static Map<String, Object> getInstrumentationScopeAttributes(final Instru
if (!instrumentationScope.getVersion().isEmpty()) {
instrumentationScopeAttr.put(INSTRUMENTATION_SCOPE_VERSION, instrumentationScope.getVersion());
}
if (!instrumentationScope.getAttributesList().isEmpty()) {
instrumentationScopeAttr.put(ATTRIBUTES_KEY, OTelProtoCodec.unpackKeyValueListLog(instrumentationScope.getAttributesList()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, the attributes should be put under instumentationsScope.attributes and filtered similar to all other attributes: Prefix + Dedotting. Won't this proposed handling add them in a very confusing location and just named attributes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am comparing with the output generated by OTEL and it looks like they are putting it under "scope" like this

"scope": {
        "name": "my.library",
        "attributes": {
          "my.scope.attribute": "some scope attribute"
        },
        "version": "1.0.0"
      },

So, it matches with it.

@kkondaka
Copy link
Collaborator Author

Thanks @KarstenSchnitter. I am not sure if we should support old version. It's been 4 years since the specification is changed

@kkondaka
Copy link
Collaborator Author

@KarstenSchnitter @dlvenable the tests are not changed (and failing ) because I wanted to get your opinion on this. Forgot to make it a draft PR.

@kkondaka
Copy link
Collaborator Author

@KarstenSchnitter #5322 is great. I can abandon this PR and approve that.

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.

2 participants