search

eclipse / microprofile-telemetry

microprofile telemetry
Apache License 2.0
19 stars 18 forks source link

Make JVM metrics properly configure themselves #240

Closed jasondlee closed 1 month ago

jasondlee commented 2 months ago

The JVM metrics tests are the only tests that don't properly configure themselves. This change adds this configuration usin MP Config like the other tests do, thus allowing the entire TCK to be run in a single surefire execution, with the only external configuration required being the log file location.

Emily-Jiang commented 2 months ago

For JVM Metrics, the runtime has to read the value prior to the app start, so the configuration was set as an environment variable or system property. The setup was documented here.

jasondlee commented 2 months ago

Which value? I have WildFly passing the TCK with these changes (as well as with the published version). Unless I'm missing something, other than pointing to the log file location, there's no need for external config.

Either way, you can still use external config, but this change makes it so you don't have to unless there's some need with a particular implementation.

jasondlee commented 1 month ago

This can be done by setting the sys props in the pom.xml of the tck. Also, Quarkus uses quarkus.otel.* prefix. The solution should allow for other prefixes to be used.

The goal of this change is to allow the tests to run while requiring a minimum of external configuration. As the change is now, that means just the log file, as there's no standard location or name for application or server logs. The property names used are those dictated by the OpenTelemetry project, as well as the MP Telemetry spec (in consuming and exposing the Otel spec more or less unchanged) and seem like reasonable defaults. If a given impl needs different prefixes, then it seems fair to expect it to configure things as needed.

Emily-Jiang commented 1 month ago

Which value? I have WildFly passing the TCK with these changes (as well as with the published version). Unless I'm missing something, other than pointing to the log file location, there's no need for external config.

All of the values mentioned in the readme. This is to configure JVM metrics. Different runtime might have different way/time to write the metrics. Setting on a wider scope is less restrictive. Either way, you can still use external config, but this change makes it so you don't have to unless there's some need with a particular implementation. Maybe using the idea @brunobat mentioned (setting in the pom.xml) is a better approach.

jasondlee commented 1 month ago

Which value? I have WildFly passing the TCK with these changes (as well as with the published version). Unless I'm missing something, other than pointing to the log file location, there's no need for external config.

All of the values mentioned in the readme. This is to configure JVM metrics. Different runtime might have different way/time to write the metrics. Setting on a wider scope is less restrictive.

Either way, you can still use external config, but this change makes it so you don't have to unless there's some need with a particular implementation. Maybe using the idea @brunobat mentioned (setting in the pom.xml) is a better approach.

I get all of that, I and I have the TCKs running and passing by setting up the POM as described in the README. What this change does is provide a happy path to running the TCK. No extra config needed. As it is, you have to the one surefire execution for the application metrics tests, and another for the JVM metrics tests. With this change, since the tests default to reasonable values, you don't need that. You can run all of the tests in a single execution. There's less to think about and less to get wrong in setting up the TCK. IF you need something different, you can still fall back to explicit extra configuration, multiple executions, etc. I fail to see why this change is controversial.

radcortez commented 1 month ago

I don't see any issue setting this configuration in the tests themselves as the TCK defaults. It seems to me that this just removes some setup effort. It is still possible to set the configuration (to override the test one) in surefire or whatever other runner used to execute the TCK.

donbourne commented 1 month ago

if the settings were provided in the pom (checked into the TCK that way), would that avoid anyone having to manually set env vars before running the test (including both those running with a server that can't see microprofile-config.properties at startup and those running with a server that can)? @brunobat , not sure if you have a counterproposal that would satisfy everyone?

jasondlee commented 1 month ago

if the settings were provided in the pom (checked into the TCK that way), would that avoid anyone having to manually set env vars before running the test (including both those running with a server that can't see microprofile-config.properties at startup and those running with a server that can)? @brunobat , not sure if you have a counterproposal that would satisfy everyone?

What's wrong with providing default values via microprofile-config.properties directly in the tests, then allow implementers to override those as needed in the build? I see no technical reason NOT to do this, as this is exactly how MP Config was designed to work.

donbourne commented 1 month ago

I'm fine with it if @brunobat is (and he said he is)... I thought he needed to change this to accommodate different prefixes, but if that's not the case, I don't have a concern.

Emily-Jiang commented 1 month ago

We discussed this in today's call. This PR allows some runtime to launch the tests without doing the global configuration. Anyway, the global configuration overwrites the app level.