align wildfly metrics with semconv#14208
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.
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.
[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.
[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.
…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.
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.
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.
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.
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