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.

Sign up for GitHub

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

Jump to bottom

[WFLY-18582] Add prometheus endpoint to micrometer extension #17857

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WFLY-18582] Add prometheus endpoint to micrometer extension #17857

wants to merge 2 commits into from

Conversation

jasondlee
Copy link
Contributor

@jasondlee jasondlee commented Apr 29, 2024 •
edited by wildfly-bot bot

https://issues.redhat.com/browse/WFLY-18582

  • Move OTLP registry configuration to a child resource

More information about the wildfly-bot[bot]

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Apr 29, 2024
@jasondlee jasondlee force-pushed the WFLY-18582-new branch 4 times, most recently from 12dcfc8 to 33185af Compare May 7, 2024 00:34
@jasondlee jasondlee marked this pull request as ready for review May 7, 2024 23:10
@jasondlee jasondlee force-pushed the WFLY-18582-new branch 3 times, most recently from f97ce30 to fd403a0 Compare May 10, 2024 20:38
@jasondlee
Copy link
Contributor Author

/retest

bstansberry
bstansberry reviewed May 16, 2024
View reviewed changes
...c/main/java/org/wildfly/extension/micrometer/MicrometerExtensionTransformerRegistration.java
@@ -0,0 +1,31 @@
package org.wildfly.extension.micrometer;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs the copyright/license header.

bstansberry
bstansberry reviewed May 16, 2024
View reviewed changes
...ter/src/main/java/org/wildfly/extension/micrometer/otlp/OtlpRegistryDefinitionRegistrar.java
@@ -0,0 +1,102 @@
package org.wildfly.extension.micrometer.otlp;
Copy link
Contributor

Choose a reason for hiding this comment

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

header

bstansberry
bstansberry reviewed May 16, 2024
View reviewed changes
...ometer/src/main/java/org/wildfly/extension/micrometer/registry/WildFlyCompositeRegistry.java
@@ -0,0 +1,95 @@
package org.wildfly.extension.micrometer.registry;
Copy link
Contributor

Choose a reason for hiding this comment

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

header

bstansberry
bstansberry reviewed May 16, 2024
View reviewed changes
...micrometer/src/test/java/org/wildfly/extension/micrometer/MicrometerTransformerTestCase.java
@@ -0,0 +1,105 @@
package org.wildfly.extension.micrometer;
Copy link
Contributor

Choose a reason for hiding this comment

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

header

bstansberry
bstansberry reviewed May 16, 2024
View reviewed changes
...ity/micrometer/src/main/java/org/wildfly/extension/micrometer/MicrometerSubsystemSchema.java Outdated
.addAttributes(MicrometerSubsystemRegistrar.ENDPOINT,
MicrometerSubsystemRegistrar.STEP)
.setXmlElementName("otlp-registry")
.build()
Copy link
Contributor

Choose a reason for hiding this comment

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

AIUI the if and else clauses do the same thing. Should the 'addChild' statement be removed from the 'else'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's a good catch. The issue here is that the model is changing, but the XML structure is not, and it looks like a little noise was left here in trying to figure out how to handle that correctly. The two branches are slightly different in the the attribute defs come from different places, though the defs themselves are identical, so that is probably a "distinction without a difference". :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, well. Not QUITE identical. The definition on MicrometerSubsystemRegistrar includes .addFlag(AttributeAccess.Flag.ALIAS) which I added, iirc, due to some runtime errors at one point related to attribute translation. That might also be noise. @pferraro ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The set of attributes of the root resource should be different between version 1.0 and 2.0, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. I'll dig into that.

bstansberry
bstansberry reviewed May 16, 2024
View reviewed changes
...ometer/src/main/java/org/wildfly/extension/micrometer/registry/WildFlyCompositeRegistry.java Outdated
// }
// }

private Meter.Id addCounter(WildFlyMetric metric, MetricMetadata metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of that class is no longer used. The class itself may be superfluous. I'll verify then do the needful.

bstansberry
bstansberry reviewed May 16, 2024
View reviewed changes
...src/test/java/org/wildfly/test/integration/observability/micrometer/MicrometerSetupTask.java
@@ -29,6 +30,8 @@ public class MicrometerSetupTask extends AbstractSetupTask {

@Override
public void setup(final ManagementClient managementClient, String containerId) throws Exception {
otelCollector = OpenTelemetryCollectorContainer.getInstance();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the 'dockerAvailable' handling being dropped? Is this because WFARQ properly handles failures from OpenTelemetryCollectorContainer.getInstance() now?

If so I believe 'dockerAvailable" can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That check is being done in the test itself:

    @BeforeClass
    public static void checkForDocker() {
        AssumeTestGroupUtil.assumeDockerAvailable();
    }

@jasondlee jasondlee changed the title [Take Two] Add prometheus endpoint to micrometer extension [WFLY-18582] Add prometheus endpoint to micrometer extension May 16, 2024
@jasondlee
[WFLY-18582-new]
ca7d087 
Introduce new model/schema version
Move OTLP-related attributes to a child resource
Update test to reflect model change
@github-actions github-actions bot added the deps-changed Dependencies have been checked, and there are changes highlighted in a comment label May 22, 2024
@github-actions GitHub Actions
Copy link

Dependency Tree Analyzer Output:

New Dependencies:

  • io.micrometer:micrometer-registry-prometheus:jar:1.12.4:compile
  • io.prometheus:simpleclient:jar:0.16.0:compile
  • io.prometheus:simpleclient_common:jar:0.16.0:compile

CC @wildfly/prod

@github-actions github-actions bot removed the deps-ok Dependencies have been checked, and there are no significant changes label May 22, 2024
@jasondlee jasondlee force-pushed the WFLY-18582-new branch 2 times, most recently from d14cb4d to 8fa5934 Compare May 22, 2024 22:23
@jasondlee
[WFLY-18582-new]
f1c1edf 
Add prometheus support
Add tests for Prometheus context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bstansberry bstansberry bstansberry left review comments

@pferraro pferraro Awaiting requested review from pferraro

Assignees
No one assigned
Labels
deps-changed Dependencies have been checked, and there are changes highlighted in a comment
Projects
None yet
Milestone
No milestone
3 participants
@jasondlee @bstansberry @pferraro