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

feat: Implement Pulsar 4.0 monitoring support with messaging.client.sent.messages #13599

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

crossoverJie
Copy link
Member

  • A new monitoring metric has been added for Pulsar 4.0: messaging.client.sent.messages.
  • Refactor common components into pulsar-common module

crossoverJie and others added 23 commits June 14, 2024 15:40

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…nstrumentation/api/incubator/semconv/messaging/MessagingMetricsAdvice.java

Co-authored-by: Lauri Tulmin <tulmin@gmail.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…nstrumentation/api/incubator/semconv/messaging/MessagingProducerMetrics.java

Co-authored-by: Lauri Tulmin <tulmin@gmail.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…nstrumentation/api/incubator/semconv/messaging/MessagingProducerMetricsTest.java

Co-authored-by: Steve Rao <raozihao.rzh@alibaba-inc.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…nstrumentation/api/incubator/semconv/messaging/MessagingProducerMetricsTest.java

Co-authored-by: Steve Rao <raozihao.rzh@alibaba-inc.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@crossoverJie crossoverJie requested a review from a team as a code owner March 27, 2025 10:18
@laurit
Copy link
Contributor

laurit commented Mar 27, 2025

Have a look at https://opentelemetry.io/docs/specs/semconv/messaging/messaging-metrics/ At the start of the document there is the following warning

Existing messaging instrumentations that are using v1.24.0 of this document (or prior):

SHOULD NOT change the version of the messaging conventions that they emit by default until the messaging semantic conventions are marked stable. Conventions include, but are not limited to, attributes, metric and span names, span kind and unit of measure.
SHOULD introduce an environment variable OTEL_SEMCONV_STABILITY_OPT_IN in the existing major version which is a comma-separated list of values. The list of values includes:
messaging - emit the new, stable messaging conventions, and stop emitting the old experimental messaging conventions that the instrumentation emitted previously.
messaging/dup - emit both the old and the stable messaging conventions, allowing for a seamless transition.
The default behavior (in the absence of one of these values) is to continue emitting whatever version of the old experimental messaging conventions the instrumentation was emitting previously.
Note: messaging/dup has higher precedence than messaging in case both values are present
SHOULD maintain (security patching at a minimum) the existing major version for at least six months after it starts emitting both sets of conventions.
SHOULD drop the environment variable in the next major version.
SHOULD emit the new, stable values for span name, span kind and similar “single” valued concepts when messaging/dup is present in the list.

Secondly the purpose of the refactor is unclear. As there is no latest dep restriction in 2.8 instrumentation it must still apply to 4.0.

@crossoverJie
Copy link
Member Author

Have a look at opentelemetry.io/docs/specs/semconv/messaging/messaging-metrics At the start of the document there is the following warning

This is a brand new metric: messaging.client.sent.messages, so there are no compatibility issues.

Secondly the purpose of the refactor is unclear. As there is no latest dep restriction in 2.8 instrumentation it must still apply to 4.0.

When the pulsar-client version in use is greater than or equal to 4.0, the instrumentation for 2.8 will still function normally; in other words, the 4.0 version of the pulsar-client only adds new metrics on top of the 2.8 version.

I have tested this locally, and it is working as expected.

@trask
Copy link
Member

trask commented Mar 27, 2025

there are no compatibility issues

this is true, but the semconv guidance is to hide new metrics and attributes behind the flag, since it represents a newer version of the semconv schema (ideally we should be populating schema url on everything, see #11939)

@crossoverJie
Copy link
Member Author

there are no compatibility issues

this is true, but the semconv guidance is to hide new metrics and attributes behind the flag, since it represents a newer version of the semconv schema (ideally we should be populating schema url on everything, see #11939)

Thank you for your additional input.

Is your opinion to handle this PR after enabling compatibility(-Dotel.semconv-stability.opt-in=messaging) for messaging semantics?

Or should we wait until the messaging semantics are stable before addressing this?  

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.

None yet

3 participants