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

More and proper tags for observation spans #2609

Closed
cmergenthaler opened this issue Mar 8, 2023 · 12 comments · Fixed by #3169
Closed

More and proper tags for observation spans #2609

cmergenthaler opened this issue Mar 8, 2023 · 12 comments · Fixed by #3169

Comments

@cmergenthaler
Copy link

cmergenthaler commented Mar 8, 2023

Expected Behavior

The current tags for tracing spans are very limited and contain only a few relevant information. Opentelemetry defines all the recommended and required attributes a single message span should include. It would be nice to have them predefined by spring-kafka automatically instead of using the customizer explicitly. Even though customizing might work, we lack a few information on the customizer to follow otel standardization properly (used broker, consumergroup, partition for producer, etc.).

Current Behavior

The only tags currently included are spring related tags (e.g. spring.kafka.template) and do not follow any standard. There are other spring projects that already have proper otel tags defined (e.g. spring-data-mongodb).

I think it would be beneficial to have consistent standard support over different spring projects.

Context

To workaround this one can implement their own KafkaTemplateObservationConvention and define the necessary tags:

public class KafkaTemplateObservationConvention extends KafkaTemplateObservation.DefaultKafkaTemplateObservationConvention {
    public static final KafkaTemplateObservation.DefaultKafkaTemplateObservationConvention INSTANCE =
            new KafkaTemplateObservationConvention();

    @Override
    public KeyValues getLowCardinalityKeyValues(KafkaRecordSenderContext context) {
        context.setRemoteServiceName(context.getBroker()); //peer.service -> should be the broker which is not available here
        return KeyValues.of(KafkaTemplateObservation.TemplateLowCardinalityTags.BEAN_NAME.asString(),
                context.getBeanName())
                .and("messaging.system", "kafka")
                .and("messaging.destination.kind", "topic")
                //.and("messaging.kafka.partition", context.getCarrier().partition().toString()) //Partition is always null
                //.and("messaging.kafka.message.offset", context.getCarrier().offset()) //Offset is not exposed
                .and("messaging.destination.name", context.getDestination());
    }

    @Override
    public String getContextualName(KafkaRecordSenderContext context) {
        return context.getDestination() + " publish";
    }
}

As you can see some required/recommended information is missing on the customizer.

@garyrussell garyrussell added ideal-for-user-contribution An issue that would ideal for a user to get started with contributing. and removed status: waiting-for-triage labels Mar 8, 2023
@garyrussell garyrussell added this to the Backlog milestone Mar 8, 2023
@garyrussell
Copy link
Contributor

Contributions are welcome.

@cmergenthaler
Copy link
Author

I'll create a PR for that soon :)

@artembilan
Copy link
Member

return context.getDestination() + " publish";

I believe previously it was send...
That's why we have it like that everywhere.

@cmergenthaler
Copy link
Author

No problem, I will just change that.

Do you know if it is possible to get the name of the broker (+ port) for the consumed/produced message somehow?

@artembilan
Copy link
Member

Well, no problem with publish!
We may accept that change as well.
Not sure if we should treat it as a breaking change since even that OpenTelemetry does not do that when they rename attributes or their recommended values.

Re. broker name. Quoting that OTEL docs:

For Apache Kafka producers, peer.service SHOULD be set to the name of the broker or service the message will be sent to. The service.name of a Consumer’s Resource SHOULD match the peer.service of the Producer, when the message is directly passed to another service.

It might be great if they give us some hint what they talk about.
There is no something like plain "broker" in Kafka: we always connect to some cluster.
See respective bootstrap.servers Kafka property to connect: https://kafka.apache.org/documentation/#producerconfigs_bootstrap.servers.

So, probably we just can take that property from config.
However according to their docs it is something arbitrary chosen by end-user: https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/span-general/#general-remote-service-attributes

@cmergenthaler
Copy link
Author

I think it does not make sense to specify all brokers from bootstrap.servers as the peer.service. Therefore I will leave it as it currently is (Apache Kafka <cluster-id>).

Maybe in the future a configurable cluster name or something would be benefical for that, from the docs: Instrumentations SHOULD provide a way for users to configure this name

@artembilan
Copy link
Member

Instrumentations SHOULD provide a way for users to configure this name

Right, so, that's probably what a KafkaTemplateObservationConvention is for.

@jonatan-ivanov
Copy link
Member

I would like to call out something if I may:
Most of the OTel semantic conventions are not stable yet (see "Status: Experimental" in the linked doc above). This means that they can change in the future: keys can be renamed, like messaging.message.abc -> messaging.message.xyz.

I think adding extra data is a great idea but we should be comfortable enough with the names to keep them even if OTel changes the specifications.

@artembilan
Copy link
Member

Thanks, @jonatan-ivanov , for feedback!
At least I see now that we are on the same page.

So, probably a middle ground could be like this:

  1. We add those new tags according to the current spec or feeling.
  2. We are OK to change them into a new spec, but only in new major or minor version.
  3. And do that only on demand and possibly via contribution.

Everyone who's eager to follow the current spec with our current version feels free to use this API:

        /**
         * Register an observation filter for the {@link Observation observations}.
         * @param observationFilter an observation filter to add to the current
         * configuration
         * @return This configuration instance
         */
        public ObservationConfig observationFilter(ObservationFilter observationFilter) {

to modify our tags to the target expectations.

Looks like the same mentioned send/publish dissonance could be achieved with this:

observationFilter(ctx -> { 
       ctx.setContextualName(ctx.getContextualName().replaceFirst("send", "publish"));
       return ctx; 
     })

Probably that Context API could be improved to the fluent calls, where we would be able just do:

observationFilter(ctx -> ctx.setContextualName(ctx.getContextualName().replaceFirst("send", "publish")))

@jonatan-ivanov
Copy link
Member

Sounds good!
Also, the ObservationConvention concept can help a lot in the future if needed.
Let's say we start using the OTel messaging conventions, release it then the convention will change in a breaking way and later becomes stable. If there will be a high enough user need, a secondary ObservationConvention can be added and users can choose if they want to use the default one that follows the older conventions (not breaking them) or the secondary one that follows the newer ones (breaking them).

@garyrussell
Copy link
Contributor

I suggested something similar here #2616 (comment)

Just provide OTEL convention implementations that the user can choose to use instead of the default ones.

@artembilan
Copy link
Member

Sure!

That's your call.
I'll be happy to review whatever you end up.

@artembilan artembilan modified the milestones: Backlog, 3.2.0-RC1 Apr 8, 2024
@artembilan artembilan removed the ideal-for-user-contribution An issue that would ideal for a user to get started with contributing. label Apr 8, 2024
artembilan pushed a commit that referenced this issue Apr 8, 2024
Fixes: #2609

* Improved observation tags to follow Opentelemetry standard (Listener)

* Otel conform tags for kafka sender

* Moved partition and offset to high-cardinality keys and revert publish -> send

* author and formatting

* * fix review.
* polish `ObservationTests`.
* add @author.
* add doc in `whats-new.adoc`.

* * fix review.
* add javadoc for `KafkaRecordReceiverContext` construct method.

* * fix review.

---------

Co-authored-by: Christian Mergenthaler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants