-
Notifications
You must be signed in to change notification settings - Fork 1k
align wildfly metrics with semconv #14208
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
Conversation
|
🔧 The result from spotlessApply was committed to the PR branch. |
|
🔧 The result from spotlessApply was committed to the PR branch. |
…va-instrumentation into jmx-wildfly
|
🔧 The result from spotlessApply was committed to the PR branch. |
…nstrumentation into jmx-wildfly
SylvainJuge
left a comment
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 be ready for review now.
| target.execInContainer("rm", "-fr", "/usr/local/tomcat/webapps/ROOT"); | ||
| target.execInContainer( | ||
| "curl", sampleWebApplicationUrl, "-o", "/usr/local/tomcat/webapps/ROOT.war"); | ||
| copyTestWebAppToTarget(target, "/usr/local/tomcat/webapps/ROOT.war"); |
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.
[for reviewer] we use a locally built app instead of downloading one from a remote URL, that removes dependency on network availability and also makes test likely more reliable.
| @ValueSource( | ||
| strings = { | ||
| // keep testing on old and deprecated version for compatibility | ||
| "jboss/wildfly:10.1.0.Final", |
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.
[for reviewer] in practice we could have tested with earlier versions but some metrics are not available with them. 10.x version is has been released ~9 years ago.
PeterF778
left a comment
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.
Looks good!
instrumentation/jmx-metrics/library/src/main/resources/jmx/rules/wildfly.yaml
Show resolved
Hide resolved
instrumentation/jmx-metrics/library/src/main/resources/jmx/rules/wildfly.yaml
Outdated
Show resolved
Hide resolved
instrumentation/jmx-metrics/library/src/main/resources/jmx/rules/wildfly.yaml
Outdated
Show resolved
Hide resolved
instrumentation/jmx-metrics/library/src/main/resources/jmx/rules/wildfly.yaml
Outdated
Show resolved
Hide resolved
instrumentation/jmx-metrics/library/src/main/resources/jmx/rules/wildfly.yaml
Outdated
Show resolved
Hide resolved
...mx-metrics/library/src/test/java/io/opentelemetry/instrumentation/jmx/rules/WildflyTest.java
Outdated
Show resolved
Hide resolved
instrumentation/jmx-metrics/testing-webapp/src/main/webapp/WEB-INF/jboss-web.xml
Show resolved
Hide resolved
...-webapp/src/main/java/io/opentelemetry/instrumentation/jmx/testapp/JakartaSimpleServlet.java
Outdated
Show resolved
Hide resolved
…nstrumentation into jmx-wildfly
| metric: created | ||
| type: counter | ||
| desc: The total number of transactions created | ||
| # wildfly.transaction.committed |
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'd prefer having wildfly.transaction.committed.count because it bescribes a data it keeps better than just wildfly.transaction.committed.
Many metrics in this file have .count suffix, but many do not, or even got it removed.
Do you think we could use .count suffix as a standard for counters/updowncounters ? Semconv recommends it for updowncounters anyway.
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.
That's something that I really considered doing, however adding the .count to every counter/updowncounter metric seems a bit redundant so I think it's simpler to use it only when needed, for example in the following cases:
wildfly.db.client.connection.wait.countadding count helps with the naming as "wait" is not really a noun, also using a similar pattern we could capture the "wait time", for example withwildfly.db.client.connection.wait.duration, in this case it would anticipate a potential future use-case that does not exist yet.wildfly.session.active.countbecause we also havewildfly.session.active.limit, however for sessions we havewildfly.session.{created,expired,rejected}: there isn't any further breakdown possible, hence it should be fine to define them without.countsuffix.
The way I understand the semconv specification here is that we could use .count to remove ambiguity or avoid plural terms, not that we have to use this strategy for every counter.
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 still not 100% convinced, but this is just my personal preference to keep metric names as explicit as possible.
If I see a metric with the name like tomcat.error.count it is obvious that we have number of errors. If I see metric with name tomcat.error it is not obvious what data is there, at least not at the first glance.
And according to the example you brought up:
wildfly.session.active.count, wildfly.session.created, wildfly.session.expired and wildfly.session.rejected share similar category of data, but do not share naming pattern.
I'd really prefer having consitent wildfly.session.{active,created,expired,rejected} or wildfly.session.{active,created,expired,rejected}.count, but I'm not going to hold this PR if nobody else has any objections.
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.
As another point of reference the JVM metrics, which happen to be the only ones that are currently part of semconv tend to only use the .count to indicate the current value, for example we have:
jvm.class.loadedjvm.class.countjvm.class.unloaded
And in the case of memory buffers we already have a case where we have "distinct depths" in the same namespace:
jvm.buffer.countjvm.buffer.memory.usedjvm.buffer.memory.limit
…nstrumentation into jmx-wildfly
…nstrumentation into jmx-wildfly
Summary of changes
Network
wildfly.network.iometricserverattribute renamed towildfly.serverlistenerattribute renamed towildfly.listenerdirectionattribute replaced withnetwork.io.directionRequests / Errors
wildfly.request.errorCountmetric renamed towildfly.error.counttomcat.error.countwildfly.request.requestCountmetric renamed towildfly.request.counttomcat.request.countserverattribute renamed towildfly.serverlistenerattribute renamed towildfly.listenerwildfly.request.processingTimemetric renamed towildfly.request.duration.sumtomcat.request.duration.sumserverattribute renamed towildfly.serverlistenerattribute renamed towildfly.listenerSessions
wildfly.session.expiredSessionmetric renamed towildfly.session.expireddeploymentattribute renamed towildfly.deploymentwildfly.session.rejectedSessionsmetric renamed towildfly.session.rejecteddeploymentattribute renamed towildfly.deploymentwildfly.session.sessionsCreatedmetric renamed towildfly.session.createddeploymentattribute renamed towildfly.deploymentwildfly.session.activeSessionsmetric renamed towildfly.session.active.counttomcat.session.active.countdeploymentattribute renamed towildfly.deploymentwildfly.session.active.limitmetric addedtomcat.session.active.limitwildfly.deploymentmetric attributeDatabase connection pool
reusing
db.client.connectionas part of the namespace to stay close to db client metrics and their attributes in semconv.wildfly.db.client.connections.usagemetric renamed towildfly.db.client.connection.count, type changed fromGaugetoUpDownCounterstateattribute replaced withdb.client.connection.statedata_sourceattribute renamed todb.client.connection.pool.namewildfly.db.client.connections.WaitCountmetric renamed towildfly.db.client.connection.wait.countdata_sourceattribute renamed todb.client.connection.pool.nameTransactions
wildfly.db.client.transaction.NumberOfTransactionsmetric renamed towildfly.transaction.createdwildfly.db.client.rollback.countmetric renamed towildfly.transaction.rollbackcauseattribute renamed towildfly.rollback.causewildfly.transaction.committedmetric added to capture the rolled-back transactionswildfly.transaction.countmetric added for the current in-flight transactions