-
Notifications
You must be signed in to change notification settings - Fork 93
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
IGNITE-17443 Implement OpenTelemetry metric exporter. #4531
base: main
Are you sure you want to change the base?
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.
It will be useful to add the DEBUG/TRACE level log at least, for the cases, when occasionally we receive unknown metric type here.
CUMULATIVE, | ||
List.of(ImmutableHistogramPointData.create( | ||
epochNanos, epochNanos, Attributes.empty(), Double.NaN, false, Double.NaN, false, Double.NaN, | ||
Arrays.stream(metric0.bounds()).asDoubleStream().boxed().collect(toUnmodifiableList()), |
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's a pretty often (every X seconds, I guess) executable piece of code. By the our https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines we are trying to ban stream APIs in this type of places.
*/ | ||
@PolymorphicConfigInstance(OtlpExporter.EXPORTER_NAME) | ||
public class OtlpExporterConfigurationSchema extends ExporterConfigurationSchema { | ||
@Value(hasDefault = true) |
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 pls add a javadoc with the time unit of this value?
if (metric instanceof IntMetric) { | ||
IntMetric intMetric = (IntMetric) metric; | ||
|
||
result.add(ImmutableMetricData.createLongGauge( |
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.
We've spent some effort to prepare the metrics infra, which is not producing significant amount of gargabe for GC. Looks like with this approach we will generate new objects on every metric push. At the same time, the list of metrics from call to call will be pretty stable. Doesn't this sdk has for example mutable APIs with prepared set of objects or anything like this?
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.
Added prepared set of wrappers under the real metrics.
implementation project(':ignite-metrics') | ||
implementation libs.jetbrains.annotations | ||
implementation libs.auto.service.annotations | ||
implementation libs.opentelemetry.exporter.otlp |
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 don't want to be annoying :), just want to clarify: we have a pretty strict policy about external deps, does this solution passed appropriate discussions?
…-17443 # Conflicts: # modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java # modules/runner/build.gradle # settings.gradle
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
The Review Checklist
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Notes