-
Notifications
You must be signed in to change notification settings - Fork 872
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
Update the semantic conventions to 1.6.0 #3524
Conversation
This was fixed in open-telemetry/opentelemetry-specification#1863 |
Hmm. do you think we should pass on the 1.6.0 release, then, and wait for 1.7.0 to update the semconv? |
I have no opinion there. Maybe you could even cherry-pick that particular commit in the generator script on top of 1.6. Also, it seems there is a instrumentation SIG + semantic convention WG set up, so there might be more changes ahead in the future. |
@@ -89,7 +89,7 @@ public final class {{class}} { | |||
public static final class {{class_name}} { | |||
{%- for member in attribute.attr_type.members %} | |||
/** {% filter escape %}{{member.brief | to_doc_brief}}.{% endfilter %} */ | |||
public static final {{ type }} {{ member.member_id | to_const_name }} = {{ print_value(type, member.value) }}; | |||
public static final {{ type }} {{ member.member_id | to_const_name | regex_replace(" ", "_") | regex_replace("^([0-9])","_$1") }} = {{ print_value(type, member.value) }}; |
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.
Let's just file an issue to revert this in 1.7.0
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.
1.7.0 is going to break a bunch of stuff that went in in 1.6.0, due to this stuff. That's why I was wondering if we should skip 1.6.0. I'm fine either way.
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.
Oh I read the linked issue and get it better now - so the NetHostConnectionSubtypeValues
would change from what I understand. Hrm - I guess we can still go for it, it's what we have the schema URLs for for better or worse
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.
I wonder if they'll create a schema for 1.7.0 that maps between the changes. And, how that will interact with the fact that we're having to actually change the keys here, since they aren't java compliant constant values.
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.
I think a schema can only handle the values.
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.
I guess that makes sense, yes. 👍🏽
I had to tweak the generator template a bit to account for some new constants that didn't map to java constant names.