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

Added info what are the new semantic attributes to be used #4840

Merged
merged 3 commits into from
Oct 18, 2022

Conversation

marcingrzejszczak
Copy link
Contributor

@marcingrzejszczak marcingrzejszczak requested a review from a team October 12, 2022 16:28
@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Base: 90.79% // Head: 90.79% // No change to project coverage 👍

Coverage data is based on head (e9fdb74) compared to base (d1a17d7).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #4840   +/-   ##
=========================================
  Coverage     90.79%   90.79%           
  Complexity     4844     4844           
=========================================
  Files           555      555           
  Lines         14438    14438           
  Branches       1405     1405           
=========================================
  Hits          13109    13109           
  Misses          910      910           
  Partials        419      419           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -166,13 +166,13 @@ public final class {{class}} {
public static final AttributeKey<String> HTTP_HOST = stringKey("http.host");

/**
* @deprecated This item has been removed as of 1.13.0 of the semantic conventions.
* @deprecated This item has been removed as of 1.13.0 of the semantic conventions. Please use {@link SemanticAttributes.NET_SOCK_PEER_ADDR} instead.
Copy link
Member

Choose a reason for hiding this comment

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

@jkwatson I briefly brought up the question of how long we retain deprecated semantic attributes here. If we go with one release cycle, then all of these will be deleted and this change is unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's a sticky wicket, isn't it. I don't particularly like that the spec doesn't deprecate but relies on the usage of the schema, which I'm sure no one actually uses. :(

Is there any harm in keeping these around longer?

Copy link
Member

Choose a reason for hiding this comment

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

Is there any harm in keeping these around longer?

AFAIK no, not really. I think we could keep them for a bit longer if it makes it easier for people to migrate off of them; we're certainly no longer using these in the instrumentation repo.

Copy link
Member

Choose a reason for hiding this comment

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

Two versions? Three versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

no idea. Let's raise it as a topic at the Thursday meeting.

Copy link
Member

Choose a reason for hiding this comment

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

added to agenda

@jkwatson
Copy link
Contributor

@marcingrzejszczak are you able to run the code generator as well, to get the actual java files up to date?

@marcingrzejszczak
Copy link
Contributor Author

Updated the code after running the code generator

@jack-berg jack-berg merged commit a3551e7 into open-telemetry:main Oct 18, 2022
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.

5 participants