-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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][misc] PIP-320: Add OpenTelemetry scaffolding #22010
[feat][misc] PIP-320: Add OpenTelemetry scaffolding #22010
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks brilliant now, thanks for fixing all the comments!
...r-otel-integration/src/main/java/org/apache/pulsar/common/stats/OpenTelemetryAttributes.java
Outdated
Show resolved
Hide resolved
pulsar-otel-integration/src/main/java/org/apache/pulsar/common/stats/package-info.java
Outdated
Show resolved
Hide resolved
...r-functions/worker/src/main/java/org/apache/pulsar/functions/worker/PulsarWorkerService.java
Show resolved
Hide resolved
...ar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/stats/package-info.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Good work @dragosvictor
...ar-opentelemetry/src/test/java/org/apache/pulsar/opentelemetry/OpenTelemetryServiceTest.java
Show resolved
Hide resolved
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #22010 +/- ##
=============================================
+ Coverage 36.56% 73.64% +37.07%
- Complexity 12418 32043 +19625
=============================================
Files 1729 1870 +141
Lines 132076 139039 +6963
Branches 14452 15245 +793
=============================================
+ Hits 48295 102394 +54099
+ Misses 77387 28712 -48675
- Partials 6394 7933 +1539
Flags with carried forward coverage won't be shown. Click here to find out more.
|
PIP-320
Motivation
PIP-264 laid out the foundation for switching our entire metrics pipeline to OpenTelemetry. PIP-320 describes the first step in this process, adding an SDK wrapper to instantiate an OpenTelemetry client with a couple of sane defaults for Pulsar brokers, proxies and function workers. This PR provides the implementation for the new wrapper.
Modifications
pulsar-otel-metrics-provider
to encapsulate the newOpenTelemetryService
. Our immediate goal is to make the service available to the broker, proxy, and function worker. Putting this in a new artifact allows us to cherry-pick this dependency as needed. Alternatively, this could be moved topulsar-common
, but it would then leak into the clients as well, which are out of scope for now. For the desired out-of-the-box experience, this artifact additionally pulls in the OTLP and Prometheus exporter dependencies.OpenTelemetryService
class as a wrapper for the safe instantiation of the OpenTelemetry SDK. It serves the following purposes:Meter
(called the cardinality limit) from 2000 to 10000.pulsar.cluster
as a resource attribute (aka label) to all metrics emitted by this SDK instance. Note that this parameter cannot be null or empty. Many of the existing Proxy tests were not setting this value in their configuration and had to be adapted. This value can also be overridden by environment variables or system properties.Meter
objects. These are used to emit the actual metrics.Verifying this change
This change added tests and can be verified as follows:
OpenTelemetryService
, including:testClusterNameCannotBeEmpty
testClusterNameCannotBeNull
testResourceAttributesAreSet
testIsInstrumentationNameSetOnMeter
testMetricCardinality
testLongCounter
: verifies basic integer counter metrics can be emittedtestServiceIsDisabledByDefault
MetricsTest
, validating the entire end-to-end metrics pipeline for brokers, proxies, and function-workers, using both the in-process Prometheus exporter and remote OTLP collector. Separated to run in its own CI integration test target, as it did not naturally fit anywhere else.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: dragosvictor#6