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

Otel metrics support #39032

Merged
merged 1 commit into from
Jul 8, 2024
Merged

Otel metrics support #39032

merged 1 commit into from
Jul 8, 2024

Conversation

brunobat
Copy link
Contributor

@brunobat brunobat commented Feb 27, 2024

Things to look at:

  • quarkus.otel.metrics.enabled=false by default at build time.
  • gRPC sender refactoring. Both the gRPC and HTTP senders are reusable between signals.
  • General refactoring, most packages and class names assumed OpenTelemetry Tracing, now we have other signals.
  • Metrics wiring using SPI-CDI bridge, not the customizers. (Still a work in progress)
  • Update the shutdown hook.
  • New Build steps and resources retention.
  • New tests in Deployment plus OpenTelemetry and Vert.x exporter IT projects.
  • CDI bean selection and exclusions on the exporter provider.
  • Metrics export injection
  • AggregationTemporality and DefaultAggregation on the metrics exporter

Includes:

  • Add recent new tls to the metrics exporter
  • Drop of deprecated properties

Ongoing:

  • Docs, on a different PR

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 27, 2024

/cc @radcortez (opentelemetry)

@ayushghosh
Copy link

@brunobat do we have any eta for this being merged ?

@brunobat
Copy link
Contributor Author

brunobat commented Mar 8, 2024

Not yet @ayushghosh

@brunobat
Copy link
Contributor Author

brunobat commented May 13, 2024

PR is ready to review. Will be working on the docs in a different PR because this one is already too big.

@brunobat brunobat marked this pull request as ready for review May 13, 2024 19:01
@brunobat brunobat requested review from geoand and radcortez May 13, 2024 19:01
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented May 14, 2024

The test failures are suspicious

@brunobat
Copy link
Contributor Author

brunobat commented May 14, 2024

The test failures are suspicious

very... Looking into it.

@quarkus-bot

This comment has been minimized.

@brunobat
Copy link
Contributor Author

ok... fixed native mode and broke the exporter... Looking into that.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@brunobat
Copy link
Contributor Author

brunobat commented Jul 2, 2024

@geoand and @radcortez can you guys review this one now? @gsmet PRs did the trick.

@quarkus-bot

This comment has been minimized.

@janhenke
Copy link

janhenke commented Jul 6, 2024

The test failures seem due to a missing docker image, maybe a rerun would succeed?

Is there a realistic chance to get this merged in time for the 3.13 release?

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Jul 8, 2024

@brunobat is this ready from your perspective?

Asking so I can start looking at it or not

@brunobat
Copy link
Contributor Author

brunobat commented Jul 8, 2024

The metrics part yes, @geoand. This PR is already too big.
From the documentation and configuration point of view I'm preparing updates at 2 levels:

  • On this PR I'll put metrics off by default, to comply with it's experimental nature. Working on it now.
  • On the documentation PR I'm adding signal independent configurations so ppl can configure all signals with the same connection properties... That requires overrides and some additional tweaks. OTel documentation also is going to be split.

@geoand
Copy link
Contributor

geoand commented Jul 8, 2024

Gotcha, thanks a lot

@geoand
Copy link
Contributor

geoand commented Jul 8, 2024

General refactoring, most packages and class names assumed OpenTelemetry Tracing, now we have other signals

This makes perfect sense, but I just want to make sure you are aware and okay with the fact this is will make backporting fixes to 3.8 harder.

import io.vertx.core.http.HttpMethod;
import io.vertx.core.tracing.TracingPolicy;

final class VertxHttpExporter implements SpanExporter {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is not needed because of the new VertxHttpSender? If so, maybe the old class can be renamed to the new one so we can retain the git history?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry... Just tried it but I'm not sure how to do this without creating a mess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, no problem

@brunobat
Copy link
Contributor Author

brunobat commented Jul 8, 2024

General refactoring, most packages and class names assumed OpenTelemetry Tracing, now we have other signals

This makes perfect sense, but I just want to make sure you are aware and okay with the fact this is will make backporting fixes to 3.8 harder.

Yes, that will be a pain, but my OCD makes me move them to a new package. Day to day operations will get increasingly confusing, especially when logging is brought in as well.

@geoand
Copy link
Contributor

geoand commented Jul 8, 2024

Sure, just wanted to make sure you were aware.

I'm +1 for this, I just had some comments

…ted tests to work with it. OTel Metrics off by default
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 8, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 6c87887.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@brunobat brunobat merged commit 2652c37 into quarkusio:main Jul 8, 2024
29 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jul 8, 2024
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants