-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Tracing API refactoring and fixes #32573
Conversation
API change check APIView has identified API level changes in this PR and created following API reviews. |
4c975cd
to
377f840
Compare
...ing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryUtils.java
Outdated
Show resolved
Hide resolved
...ing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryUtils.java
Outdated
Show resolved
Hide resolved
...ing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryUtils.java
Outdated
Show resolved
Hide resolved
...ing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryUtils.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/ExpandableStringEnum.java
Outdated
Show resolved
Hide resolved
return null; | ||
} catch (Throwable e) { | ||
throw new RuntimeException(e); |
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.
nit: We should inspect the type of Throwable, if this is an Error we shouldn't wrapped it and just re-throw it. Also if the Throwable is already a RuntimeException there is no need to wrap it either
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.
Throwable
comes from MethodHandle.invoke
definition and it's the other way around - RuntimeException
is Throwable
so I have to wrap it or log and return null. Previously we returned null if something didn't go well, so I switched back to this behavior and added some logs.
6c8adcc
to
2ed4f7a
Compare
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.
After this PR we should do a performance test on creating ExpandableStringEnums in a hot loop to verify there is no major regressions
Did a quick benchmark:
@Benchmark
public void expandableOld(Blackhole blackhole) {
blackhole.consume(Expandable.fromString_vOld(UUID.randomUUID().toString()));
}
@Benchmark
public void expandableNew(Blackhole blackhole) {
blackhole.consume(Expandable.fromString(UUID.randomUUID().toString()));
} |
* Tracing: support configuration and versioning and API cleanup
- Fixes build break running perf tests on older package versions caused by Azure/azure-sdk-for-java#32573
Fixes: #24086, #28953, (partially) #21686 as a preparation for tracing plugin GA (#30557)
Tracing perf impact (core perf tests):
small files:
tracing --binary-data-source bytes --parallel 32 --size 10240 --include-pipeline-policies -d 60
small blobs:
tracing --binary-data-source blobs--parallel 32 --size 10240 --include-pipeline-policies -d 60
big files (10485760 bytes)
big blobs (10485760 bytes) - no impact detected
Prototyped changes for ServiceBus, Cosmos, and Spring Sleuth: https://github.com/lmolkova/azure-sdk-for-java/tree/tracing-versioning