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

CXF-8817: Micrometer Observation #1346

Merged
merged 14 commits into from
Aug 14, 2023
Merged

Conversation

marcingrzejszczak
Copy link
Contributor

@marcingrzejszczak marcingrzejszczak commented Jul 19, 2023

This PR adds support for Micrometer Observation instrumentation of Apache CXF.

NOTE: This PR is heavily influenced by Brave's implementation.

Dependencies

  • Adds micrometer tracing
  • Adds zipkin sender

CXF metrics project

  • Now also does tracing (TODO: maybe this should be done in a separate module?)
  • Adds dependency on rt-management since metrics will now also be related to tracing
  • Adds dependency to Micrometer Observation
  • Is a copy of the Brave Tracing module but adopted to Micrometer Observation API
  • Uses low and high cardinality tags basing on OpenTelemetry Semantic Conventions for RPC and HTTP (JaxRs) from 19.07.2023. VERY IMPORTANT: This doesn't mean that this project is OTel Semantic Convention compliant!!!
  • Uses ObservationDocumentation that using https://micrometer.io/docs/observation#_documentation_generation could generate asciidoctor documentation of observability data

Spring Boot

  • adds MicrometerObservationAutoConfiguration that registers Features as beans

Tests

  • reused existing Brave tests with Micrometer Observation, altered existing tagging and naming defaults to make the test pass with Brave tagging and naming defaults
  • reused existing Boot tests to additionally assert observation based metrics and traces

TODO ? :

  • maybe the code shouldn't be in CXF metrics module but a completely separate one?
  • add Javadocs?
  • add documentation - where should it be added?

Screenshots from Zipkin:

Screenshot from 2023-07-19 13-25-48

Screenshot from 2023-07-19 13-25-55

Fixes https://issues.apache.org/jira/browse/CXF-8817

@reta
Copy link
Member

reta commented Jul 23, 2023

Thanks a lot for the pull request @marcingrzejszczak !

Now also does tracing (TODO: maybe this should be done in a separate module?)

This is the way CXF structures the integrations: the tracing is separated from metrics (integration/tracing). Could you please split them into separate modules? Thank you.

@marcingrzejszczak
Copy link
Contributor Author

@reta I've moved the Micrometer Observation integration under the tracing folder. The integration allows you to actually not have tracing at all and in essence is tracing agnostic, however for the sake of consistency I put it under the tracing folder

@reta
Copy link
Member

reta commented Jul 24, 2023

@reta I've moved the Micrometer Observation integration under the tracing folder. The integration allows you to actually not have tracing at all and in essence is tracing agnostic, however for the sake of consistency I put it under the tracing folder

Thanks a lot @marcingrzejszczak , there are some failures in the build, could you please take a look? thank you

Caused by: java.lang.IllegalStateException: Failed to introspect Class [org.apache.cxf.spring.boot.autoconfigure.micrometer.MicrometerObservationAutoConfiguration] from ClassLoader [jdk.internal.loader.ClassLoaders$AppClassLoader@5cb0d902]
	at org.springframework.util.ReflectionUtils.getDeclaredMethods(ReflectionUtils.java:483)
	at org.springframework.util.ReflectionUtils.doWithMethods(ReflectionUtils.java:360)

@marcingrzejszczak
Copy link
Contributor Author

Hey @reta . I've applied all the suggestions and rebased the branch against main. Anything else you would like me to do?

@marcingrzejszczak
Copy link
Contributor Author

LGTM!

@reta reta changed the title Micrometer Observation CXF-8817: Micrometer Observation Aug 14, 2023
@reta reta merged commit c9c962b into apache:main Aug 14, 2023
3 checks passed
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.

2 participants