-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Initial Observability extension #36470
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e8333e9
to
f69f141
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
||
@ConfigGroup | ||
@DevTarget("io.quarkus.observability.devresource.victoriametrics.VictoriaMetricsResource") | ||
public interface VictoriaMetricsConfig extends ContainerConfig { |
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 know that the project started with VictoriaMetrics, but I would remove this for a later PR. It adds to much complexity now.
We wouldn't really need PromQL and all it's needed to support it.
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.
Afais, Prometheus is up high there in the wish list, and VM == Prometheus.
I'm planning on adding real Prometheus asap.
And as such, I would leave PromQL -- it might come useful to some users.
Might probably need some more tests, but REST API was "copy/pasted" (and implemented) from the docs.
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'm not contesting being here at some point, I'm just concerned about the size of the PR. We need to break this up because the important parts are totally obfuscated.
I strongly believe we should have only Jaeger-all and Grafana-all on this first PR.
Also believe that expanding the number of destinations is secondary and we should focus on making sure the user experience is spot on.
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
<modelVersion>4.0.0</modelVersion> | ||
<parent> | ||
<artifactId>quarkus-observability-testlibs</artifactId> |
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.
Rename from testlibs to observability-devresource?
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.
There might be other test libs (artifacts) in the future, not just devresources ...
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 just because the name is very misleading... It's not just for tests, I imagine.
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.
Yeah. OK, will try to think of a better name.
quarkus.micrometer.export.otlp.enabled=true | ||
quarkus.micrometer.export.otlp.publish=true | ||
quarkus.micrometer.export.otlp.default-registry=true | ||
quarkus.micrometer.export.otlp.url=http://${quarkus.otel-collector.http}/v1/metrics |
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.
Should it probably be the other way around? Instead of quarkus.otel-collector.http
, should we pick up the value from quarkus.micrometer.export.otlp.url
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.
Hmmm, afais, no -- you need the location / root url of OTel Collector.
<dependency> | ||
<groupId>io.quarkus</groupId> | ||
<artifactId>quarkus-observability-victoriametrics</artifactId> | ||
</dependency> | ||
<!-- Add these deps as non-test / plain dependency, so we can run dev mode to check --> | ||
<dependency> | ||
<groupId>io.quarkus</groupId> | ||
<artifactId>quarkus-observability-devresource-victoriametrics</artifactId> | ||
</dependency> | ||
<dependency> | ||
<groupId>io.quarkus</groupId> | ||
<artifactId>quarkus-observability-devresource-grafana</artifactId> | ||
</dependency> |
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 think we should simplify the setup for the users. Having just quarkus-observability
already including a default devservice would be optimal.
We could create a different artifact for other options... But just a single dependency.
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.
Yeah, it's on my todo list - as we discussed - I'll add an "uber" pom, with all "normal" dependecies.
e.g. all probably don't make sense, as some exclude each other
I'd suggest to reduce the PR size so we can better iron out how users are going to use this.
|
This comment has been minimized.
This comment has been minimized.
6a16a97
to
305c4ee
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Failing Jobs - Building 0f5c776
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 17 #- Failing: integration-tests/observability-multiapp
📦 integration-tests/observability-multiapp✖
⚙️ JVM Tests - JDK 21 #- Failing: integration-tests/observability-multiapp
📦 integration-tests/observability-multiapp✖
|
Closing in favor of a new, similar PR, but (hopefully) lighter version. |
Fixes #26445
This adds dynamic lookup of possible
observability
dev resources / services.It also adds REST clients for PromQL (Prometheus query language) and VictoriaMetrics REST api support; push, etc
The dev resources / services can be run in 3 different ways
QuarkusTestResource
dev resource(1) can be configured
(2) depends what's available on the classpath
(3) needs to be explicitly used in QuarkusTestResource
Currently supported dev services
It should be easy to add new / other
observability
dev services in the future,e.g. currently missing Prometheus, etc
by adding a new DevResourceLifecycleManager implementation.
Current TODO list