-
Notifications
You must be signed in to change notification settings - Fork 874
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
Semconv 1.25.0 migration #10983
Semconv 1.25.0 migration #10983
Conversation
7787da4
to
f2d90cf
Compare
* This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
* any time. | ||
*/ | ||
public final class NetworkAttributes { |
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.
[for reviewer] I don't know the reason this class was introduced in the first place, but given all the attributes are now part of semconv, it's probably a good time to use them.
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.
As far as I know this class was temporarily introduced to use attributes that weren't present int the latest semconv jar. It is fine to remove it now.
@@ -162,12 +162,12 @@ class SpanAssert { | |||
|
|||
def errorEvent(Class<Throwable> errorClass, expectedMessage, int index) { | |||
event(index) { | |||
eventName(SemanticAttributes.EXCEPTION_EVENT_NAME) | |||
eventName("exception") |
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.
[for reviewer] this is one of the cases where the SemanticAttributes.EXCEPTION_EVENT_NAME
has no equivalent in the new semconv and I had to inline the value as-is.
If every project that depends on semconv java is impacted, then it could make sense to provide such "compatibility layer" directly in https://github.com/open-telemetry/semantic-conventions-java/ as a separate artifact to make migration easier (with of course deprecation warnings), then drop it after a few releases to push migration and avoid having to keep it forever, or we could even include it as part of the |
assertThat(resource.getAttribute(ResourceAttributes.OS_TYPE)) | ||
.isEqualTo(ResourceAttributes.OsTypeValues.LINUX); | ||
assertThat(resource.getAttribute(ResourceAttributes.OS_DESCRIPTION)).isNotEmpty(); | ||
assertThat(resource.getSchemaUrl()).isEqualTo(SchemaUrls.V1_24_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.
[for reviewer] I am not sure what is the "right" value to pick here, so I picked the 1.24.0 to match the semconv version.
💯
I'd support copying the old semconv classes in (seems easiest) and marking them all as deprecated. We can remove the copies in 3.0 when we bump database and RPC semconv. |
Following yesterday sig meeting discussion, I've opened open-telemetry/semantic-conventions-java#62 to include a copy of the old classes and that will likely be included in the upcoming 1.25.0 release of semantic-conventions-java. So such big-bang migration won't be needed once 1.25.0 is out, but the migration needs to happen anyway and it's already done here, thus I think it's still relevant to review and merge it. |
just holding off on merging this for the next semconv java 1.25.0 release, so we can avoid breaking any Java agent extensions that were relying on the old classes |
semconv java 1.25.0 has been released @SylvainJuge when you have time, can you update to 1.25.0 and resolve conflicts, then we'll merge it! |
…nstrumentation into semconv-update
509fdf2
to
6839613
Compare
Fixing the conflicts was the easy and fun part, bumping semconv to 1.25.0 also brings the following:
So with those new additions that should be good to go. |
The 1.24.0 release of semconv for java brings breaking changes that prevent a simple upgrade PR like #10971 to work as expected.
As there are lots of changes I've split the PR in separate commits:
For
SemanticAttributes.EXCEPTION_EVENT_NAME
I had to inline and duplicate the"exception"
value as there isn't any equivalent in the new semantic conventions.Follow-up questions where input is welcome
Answer is "YES" and this was done
Answer is "NO", it remains as part of the "stable" semconv artifact due to the usage of java modules that prevent having classes from a package spread in more than one artifact.
Follow-up tasks once this is merged