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

[test][broker] Exposing metrics in BrokerInterceptor #22781

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

crossoverJie
Copy link
Member

PIP: PIP-264

Motivation

Shows how to expose metrics in BrokerInterceptor.

Modifications

Using OTel in BrokerInterceptor to expose metrics.

Verifying this change

  • Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: crossoverJie#26

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 25, 2024
@crossoverJie
Copy link
Member Author

@asafm PTAL, thanks.

.put(OpenTelemetryAttributes.PULSAR_TOPIC, topicName.getPartitionedTopicName());
var attributes = builder.build();

messageCounter.add(1, attributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since many will record topic-level metrics, we need a common infra to allow you get an AttributesBuilder already filled with the topic level attributes.
@dragosvictor is currently working on implementing many OTel metrics, I suggest you get his plan for this, so you won't overlap.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a demo, it will actually be a custom attribute.

messageCounter = meter
.counterBuilder(MESSAGE_COUNTER)
.setUnit("{message}")
.setDescription("The total number of messages produced.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I get the full intent here, as this metric is in a test.

If you want to learn how to add a metric it's best by doing it for a real metric.

Copy link
Member Author

Choose a reason for hiding this comment

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

This just show how to customize a metric in an BrokerInterceptor.

In fact, this will be a business-customized metric, and the MESSAGE_COUNTER here is just a demo.

The background comes from this #20772

Copy link
Contributor

@asafm asafm Jun 1, 2024

Choose a reason for hiding this comment

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

Got it. Thanks.
So if I understand correctly you would like to have the ability to add your own metrics when you write your own BrokerInterceptor.

So this is not a real PR, but a Draft PR - I would convert it to such, so it won't by mistake gets merged.

The same use case you described exists for any "plugin" people can add to the broker: Function, Offloader plugin, Authentication plugin and many more.

As I wrote in the parent PIP of OTel (pip-264), each such plugin should create their own meter, which reflect their own namespace.

Copy link
Member Author

@crossoverJie crossoverJie Jun 2, 2024

Choose a reason for hiding this comment

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

So this is not a real PR, but a Draft PR - I would convert it to such, so it won't by mistake gets merged.

Yes, you are right.

I think this PR is also necessary; this PR can verify that using pulsarService.getOpenTelemetry().getMeter() in a BrokerInterceptor to expose metrics is as expected.

@crossoverJie crossoverJie requested a review from asafm May 26, 2024 05:25
@crossoverJie crossoverJie changed the title [test][broker] PIP-264: BrokerInterceptor support OTel [test][broker] Exposing metrics in BrokerInterceptor Jun 2, 2024
@asafm asafm marked this pull request as draft June 6, 2024 19:56
@asafm asafm marked this pull request as draft June 6, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants