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.

Sign up for GitHub

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

Jump to bottom

23458 tel shim #25409

Merged
merged 3 commits into from
Jun 14, 2023
Merged

23458 tel shim #25409

merged 3 commits into from
Jun 14, 2023

Conversation

benjamin-confino
Copy link
Contributor

Fixes #23458
Fixes #25368 which was discovered when writing this test.

Azquelt
Azquelt reviewed Jun 5, 2023
View reviewed changes
dev/io.openliberty.microprofile.telemetry.1.0.internal_fat/bnd.bnd Outdated
io.opentelemetry.instrumentation:opentelemetry-instrumentation-annotations;version=1.19.0.alpha,\
io.opentracing:opentracing-api;version=0.33.0
Copy link
Member

Choose a reason for hiding this comment

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

I do not think you need these two because the relevant classes should also be in io.openliberty.mpTelemetry.1.0.thirdparty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed opentelemetry-instrumentation-annotations, but removing opentracing-api gave local compile errors.

Azquelt
Azquelt reviewed Jun 5, 2023
View reviewed changes
dev/io.openliberty.microprofile.telemetry.1.0.internal_fat/build.gradle Outdated

dependencies {
requiredLibs project(':io.openliberty.mpTelemetry.1.0.thirdparty')
}
shimLibs 'io.opentelemetry:opentelemetry-api:1.19.0',
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you want the opentelemetry-api in shimLibs

Although the shim does depend on the otel API, as far as I can see you're using shimLibs to add the libraries to the test application. The test application shouldn't need the otel API since that's provided for it by liberty.

Azquelt
Azquelt reviewed Jun 5, 2023
View reviewed changes
....internal_fat/fat/src/io/openliberty/microprofile/telemetry/internal_fat/apps/shim/POJO.java Outdated
Comment on lines 32 to 37
@WithSpan
public void annotatedClassMethodImplicitlyTraced(Tracer tracer) {
System.out.println("Called annotatedClassMethodImplicitlyTraced");
Span span = tracer.activeSpan();
Assert.assertNotNull(span);
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be better if we could test a little more here.

In this test can we check that the spanId returned by the opentracing shim is the same as the one returned by OpenTelemetry?

It would be better if we had a test which called the opentracing API a bit more, perhaps creating a span and seeing that it is exported correctly using InMemorySpanExporter.

@benjamin-confino
Copy link
Contributor Author

#build

Azquelt
Azquelt previously approved these changes Jun 5, 2023
View reviewed changes
Copy link
Member

@Azquelt Azquelt left a comment

Choose a reason for hiding this comment

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

I would like the testing here to be a little more thorough, but we can do that in a follow up PR. The work that is here is good 👍

@LibbyBot
Copy link

LibbyBot commented Jun 5, 2023

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_ybmjQAO-Ee6Gnsx4pF5sWA

Target locations of links might be accessible only to IBM employees.

@benjamin-confino
Copy link
Contributor Author

#build

@LibbyBot
Copy link

LibbyBot commented Jun 7, 2023

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_HOgq0AUaEe6Gnsx4pF5sWA

Target locations of links might be accessible only to IBM employees.

@benjamin-confino
Copy link
Contributor Author

#build

@LibbyBot
Copy link

LibbyBot commented Jun 7, 2023

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_TjHZkAUmEe6Gnsx4pF5sWA

Target locations of links might be accessible only to IBM employees.

@benjamin-confino
Copy link
Contributor Author

#build

@LibbyBot
Copy link

LibbyBot commented Jun 8, 2023

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_kjKuQAX3Ee6Gnsx4pF5sWA

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

LibbyBot commented Jun 9, 2023

The build benjamin-confino-25409-20230608-0917
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_kjKuQAX3Ee6Gnsx4pF5sWA
completed and has errors or failures.

For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_kjKuQAX3Ee6Gnsx4pF5sWA

@benjamin-confino benjamin-confino force-pushed the 23458-tel-shim branch 4 times, most recently from b75320f to 1034d19 Compare June 12, 2023 10:24
Azquelt
Azquelt reviewed Jun 12, 2023
View reviewed changes
...1.0.internal/src/io/openliberty/microprofile/telemetry/internal/cdi/WithSpanInterceptor.java Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Copyright date 2022, 2023

Azquelt
Azquelt reviewed Jun 12, 2023
View reviewed changes
dev/io.openliberty.microprofile.telemetry.1.0.internal_fat/build.gradle Outdated
* Copyright (c) 2022 IBM Corporation and others.
* Copyright (c) 2023 IBM Corporation and others.
Copy link
Member

Choose a reason for hiding this comment

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

Should be 2022, 2023

@benjamin-confino benjamin-confino force-pushed the 23458-tel-shim branch 2 times, most recently from de460d5 to bcd24fd Compare June 12, 2023 11:05
@benjamin-confino
Copy link
Contributor Author

#build

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_8LiGYAnLEe6txPsqA1YBCw

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

The build benjamin-confino-25409-20230613-0330
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_8LiGYAnLEe6txPsqA1YBCw
completed and has errors or failures.

For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_8LiGYAnLEe6txPsqA1YBCw

@benjamin-confino
Copy link
Contributor Author

#libby

@benjamin-confino
Copy link
Contributor Author

#run-libby-bot

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.

  • 3 product code files were changed.

  • Please describe in a separate comment how you tested your changes.

  • 9 FAT files were changed, added, or removed.

  • Check that the build did not break the affected FAT suite(s).

@benjamin-confino benjamin-confino merged commit 77391c1 into OpenLiberty:integration Jun 14, 2023
@benjamin-confino benjamin-confino mentioned this pull request Jun 14, 2023
Merged
@LibbyBot LibbyBot mentioned this pull request Jun 14, 2023
Closed
@LibbyBot
Copy link

The build benjamin-confino-25409-20230615-0829
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_L8OPoguHEe6txPsqA1YBCw
completed and has errors or failures.

For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_L8OPoguHEe6txPsqA1YBCw

@LibbyBot
Copy link

The build benjamin-confino-25409-20230615-1638
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_QPl78AvMEe6txPsqA1YBCw
completed successfully!

@benjamin-confino benjamin-confino deleted the 23458-tel-shim branch September 20, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Azquelt Azquelt Azquelt approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GlobalOpenTelemetry is missing public methods Test Telemetry with the OpenTracing shim
3 participants
@benjamin-confino @LibbyBot @Azquelt