-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-49241][CORE] Add OpenTelemetryPush
Sink with opentelemetry
profile
#47763
Conversation
OpenTelemetryPush
Sink
Thank you for making a PR, @TQJADE . |
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.
Could you make CIs happy?
459fec6
to
5ad317b
Compare
|
core/pom.xml
Outdated
<dependency> | ||
<groupId>io.opentelemetry</groupId> | ||
<artifactId>opentelemetry-sdk</artifactId> | ||
<version>1.40.0</version> |
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.
Let's use the latest one.
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.
Updated
OpenTelemetryPush
SinkOpenTelemetryPush
Sink with opentelemetry
profile
pollInterval: Int = 10, | ||
pollUnit: TimeUnit = TimeUnit.SECONDS, | ||
host: String = "http://localhost", | ||
port: String = "4317", |
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.
Could you double-check this? It seems that OpenTelemetry changed this. What is the current status?
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.
@dongjoon-hyun 4317 is port for grpc and 4318 is for HTTP/HTTPS. Should I rename the port to grpcPort
?
https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/
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.
To @TQJADE , you are using http://
in line 39, aren't you? Are you using grpc
instead of HTTP here?
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.
https://github.com/open-telemetry/opentelemetry-java/blob/7522bfe141524d52649b5c344c643104b57af608/exporters/common/src/main/java/io/opentelemetry/exporter/internal/ExporterBuilderUtil.java#L32-L47
Here there is a endpoint validation, it required the endpoint to start with http/https
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.
What does that mean? Do you mean OpenTelemetry
requires HTTP
with 4317 ? If we need to use 4317
with HTTP, which HTTP call use 4318 port?
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.
I tested with grpc endpoint, when I tested, the endpoint should be started with http
or https
. The default value of Opentelmetry GRPC endpoint port is 4317. HTTP endpoint port is 4318.
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.
It seems that we are repeating the same questions and answers again and again. :)
What I asked to confirm is that why we are not using the default value of HTTP endpoint port (4318) here when the endpoint starts with http
. This PR uses http://localhost:4317
instead of http://localhost:4318
, doesn't it? We know that your sentence is correct, but it sounds like contradiction. We need an explanation for this contradiction.
the endpoint should be started with http or https. The default value of Opentelmetry GRPC endpoint port is 4317. HTTP endpoint port is 4318.
|
||
import org.apache.spark.SparkFunSuite | ||
|
||
class OpenTelemetryPushSinkSuite extends SparkFunSuite with PrivateMethodTester { |
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.
This looks like having a test coverage for only object OpenTelemetryPushSink
.
Could you add more test coverage for class OpenTelemetryPushSink
?
|
||
import org.apache.spark.SparkFunSuite | ||
|
||
class OpenTelemetryPushReporterSuite extends SparkFunSuite with PrivateMethodTester { |
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.
Could you add more test coverage?
core/pom.xml
Outdated
<dependency> | ||
<groupId>com.squareup.okhttp3</groupId> | ||
<artifactId>okhttp</artifactId> | ||
<version>3.13.0</version> |
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.
Please note that Apache Spark's okhttp
is 3.12.12. So, we had better use 3.12.12 even for the test case in order to make it sure that there is no problem inside Apache Spark 4.0.0 release.
spark/dev/deps/spark-deps-hadoop-3-hive-2.3
Line 226 in 9fc1e05
okhttp/3.12.12//okhttp-3.12.12.jar |
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.
Thank you for updating this PR. We still need your replies for the following review comments.
core/pom.xml
Outdated
@@ -651,7 +651,8 @@ | |||
<dependency> | |||
<groupId>com.squareup.okhttp3</groupId> | |||
<artifactId>okhttp</artifactId> | |||
<version>3.13.0</version> | |||
<!--version should be same with https://github.com/apache/spark/blob/0895471ad3b2a82668ea7e83dd20dda6b17f145f/dev/deps/spark-deps-hadoop-3-hive-2.3#L226--> |
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.
Please remove this comment.
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.
Removed
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.
+1, LGTM. Thank you, @TQJADE .
Merged to master.
Is it necessary to activate the also cc @panbingkun |
Got it. Actually, this PR follows the existing To @TQJADE , could you find a better way to mitigate the above situation? |
QQ: Cant this not live outside of Apache Spark - and use the existing integrations in MetricSystem ? +CC @dongjoon-hyun |
cc @dongjoon-hyun @TQJADE In fact, due to our use of I have submitted a pr: #47986. Before supplementing the PR description, I will take some time to conduct additional verification.
Of course, this question also requires a clear answer :) |
@mridulm @LuciferYang
That is the reason I would like to contribute to Apache Spark side. The OpenTelemetryPushSink is similar to StatsdSink, to push the metrics to target services. |
I am trying to understand whether there are any limitations due to which this integration has to be included as part of Apache Spark. |
Your point is correct, @mridulm .
Let me revert this commit. Thank you for all your valuable reviews, @mridulm , @LuciferYang . Personally, sorry for wasting your time, @TQJADE . |
This is reverted via 628cd75 |
Thanks for the details @dongjoon-hyun ! Really appreciate it. |
… profile ### What changes were proposed in this pull request? OpenTelemetryPushSink and OpenTelemetryPushReporter has been added in the supported Sink Classes. ### Why are the changes needed? - From OpenTelemetry side, they have added the support from a pull model: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/apachesparkreceiver - However, for a very short jobs, only the push model is applicable. Therefore, a new sink OpenTelemetryPushSink and a new reporter OpenTelemetryPushReporter have been introduced here. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? * Build the Apache Spark Core Snapshot. Reference the Snapshot and giving the local OpenTelemetry endpoint `127.0.0.1:4317` as the endpoint for OpenTelemetryReporter. * Start the open-telemetry collector locally, per: https://opentelemetry.io/docs/collector/quick-start/#generate-and-collect-telemetry ``` docker run \ -p 127.0.0.1:4317:4317 \ -p 127.0.0.1:55679:55679 \ otel/opentelemetry-collector:0.107.0 \ 2>&1 | tee collector-output.txt # Optionally tee output for easier search later ``` * Start Spark Applications and publishing the metrics * Observing the metrics to be printed. ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#47763 from TQJADE/sink. Authored-by: Qi Tan <qi_tan@apple.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
… profile ### What changes were proposed in this pull request? OpenTelemetryPushSink and OpenTelemetryPushReporter has been added in the supported Sink Classes. ### Why are the changes needed? - From OpenTelemetry side, they have added the support from a pull model: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/apachesparkreceiver - However, for a very short jobs, only the push model is applicable. Therefore, a new sink OpenTelemetryPushSink and a new reporter OpenTelemetryPushReporter have been introduced here. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? * Build the Apache Spark Core Snapshot. Reference the Snapshot and giving the local OpenTelemetry endpoint `127.0.0.1:4317` as the endpoint for OpenTelemetryReporter. * Start the open-telemetry collector locally, per: https://opentelemetry.io/docs/collector/quick-start/#generate-and-collect-telemetry ``` docker run \ -p 127.0.0.1:4317:4317 \ -p 127.0.0.1:55679:55679 \ otel/opentelemetry-collector:0.107.0 \ 2>&1 | tee collector-output.txt # Optionally tee output for easier search later ``` * Start Spark Applications and publishing the metrics * Observing the metrics to be printed. ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#47763 from TQJADE/sink. Authored-by: Qi Tan <qi_tan@apple.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
… profile ### What changes were proposed in this pull request? OpenTelemetryPushSink and OpenTelemetryPushReporter has been added in the supported Sink Classes. ### Why are the changes needed? - From OpenTelemetry side, they have added the support from a pull model: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/apachesparkreceiver - However, for a very short jobs, only the push model is applicable. Therefore, a new sink OpenTelemetryPushSink and a new reporter OpenTelemetryPushReporter have been introduced here. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? * Build the Apache Spark Core Snapshot. Reference the Snapshot and giving the local OpenTelemetry endpoint `127.0.0.1:4317` as the endpoint for OpenTelemetryReporter. * Start the open-telemetry collector locally, per: https://opentelemetry.io/docs/collector/quick-start/#generate-and-collect-telemetry ``` docker run \ -p 127.0.0.1:4317:4317 \ -p 127.0.0.1:55679:55679 \ otel/opentelemetry-collector:0.107.0 \ 2>&1 | tee collector-output.txt # Optionally tee output for easier search later ``` * Start Spark Applications and publishing the metrics * Observing the metrics to be printed. ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#47763 from TQJADE/sink. Authored-by: Qi Tan <qi_tan@apple.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What changes were proposed in this pull request?
OpenTelemetryPushSink and OpenTelemetryPushReporter has been added in the supported Sink Classes.
Why are the changes needed?
From OpenTelemetry side, they have added the support from a pull model: https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/apachesparkreceiver
However, for a very short jobs, only the push model is applicable. Therefore, a new sink OpenTelemetryPushSink and a new reporter OpenTelemetryPushReporter have been introduced here.
Does this PR introduce any user-facing change?
No
How was this patch tested?
127.0.0.1:4317
as the endpoint for OpenTelemetryReporter.Was this patch authored or co-authored using generative AI tooling?
No