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

Support micrometer-tracing #205

Merged
merged 2 commits into from
May 22, 2023
Merged

Conversation

be-hase
Copy link
Member

@be-hase be-hase commented May 16, 2023

micrometer-tracing is now available.
https://micrometer.io/docs/tracing

It may be a minor library at present, but with the support of micrometer-tracing from spring boot3, demand may increase in the future.

micrometer-tracing is a facade of brave and opentelemetry-java.
Therefore, if decaton supports micrometer-tracing, opentelemetry-java can also be supported.
(I think opentelemetry-java users will be happy)

  • ToDo
    • test code (If you are interested in micrometer-tracing supporing, I'll do)

@CLAassistant
Copy link

CLAassistant commented May 16, 2023

CLA assistant check
All committers have signed the CLA.

@be-hase be-hase force-pushed the micrometer-tracing branch 2 times, most recently from ae599b1 to 71e0393 Compare May 16, 2023 07:08
@mauhiz
Copy link
Contributor

mauhiz commented May 16, 2023

typo in commit/title : micromater -> micrometer

@be-hase be-hase changed the title Support micromater-tracing Support micrometer-tracing May 17, 2023
@be-hase be-hase force-pushed the micrometer-tracing branch from 71e0393 to 639f5e4 Compare May 17, 2023 01:04
@be-hase be-hase force-pushed the micrometer-tracing branch from 639f5e4 to 5de5950 Compare May 17, 2023 03:04
@be-hase
Copy link
Member Author

be-hase commented May 17, 2023

Added integration test code.
And, I also implemented an example application.
https://github.com/be-hase/decaton-micrometer-tracing-example

Copy link
Contributor

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!
I haven't known micrometer provides a tracing facade.

Almost looks good. Left few comments and questions.

// Since the context of parent is injected in Callback of `producer.send`, create the span manually.
// ref: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/8a15975dcacda48375cae62e98fe7551fb192d1f/instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/src/main/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/KafkaTelemetry.java#L262-L264
@Override
public Future<RecordMetadata> send(ProducerRecord<K, V> record, Callback callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is my understanding below correct?

  • As you mentioned, otel KafkaTelemetry executes producer callback inside the parent context, which no traceId is associated unless we explicitly provide some
  • ProcessingGuarantee's onProduce is executed in producer callback. Hence, we need to create a span manually to supply some traceId

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, right.

.run();
}

private static class WithParentSpanProducer<K, V> implements Producer<K, V> {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a convenient abstract class com.linecorp.decaton.testing.processor.ProducerAdaptor for avoiding implementing all producer methods with just delegating to the underlying producer.
Let's make it public and use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed: c2e5189

import io.opentelemetry.instrumentation.kafkaclients.v2_6.KafkaTelemetry;
import io.opentelemetry.sdk.OpenTelemetrySdk;

public class MicrometerTracingOtelBridgeTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way do we need to add separate integration tests for each tracer impls?

I think testing with just one tracer impl (to assert the facade usage) is sufficient from Decaton's point of view, because Decaton's responsibility is just using micrometer's tracing facade correctly, so I don't think checking if traces are actually propagated in each tracer is meaningful.

Copy link
Member Author

Choose a reason for hiding this comment

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

As you say, from decaton's point of view, it may be enough to test with one tracer.
(It is the responsibility of micrometer-tracing to verify operation with multiple tracers.)

So, shall I delete MicrometerTracingBraveBridgeTest ?
Because it is similar to the decaton-brave module.

Copy link
Contributor

Choose a reason for hiding this comment

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

shall I delete MicrometerTracingBraveBridgeTest

Sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed: c2e5189

Copy link
Contributor

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@ocadaruma ocadaruma added the new feature Add a new feature label May 22, 2023
@ocadaruma ocadaruma merged commit 5f52fe8 into line:master May 22, 2023
@be-hase be-hase deleted the micrometer-tracing branch May 22, 2023 05:15
@be-hase
Copy link
Member Author

be-hase commented May 22, 2023

thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants