Change from _app and _scope to mp_app and mp_scope #717
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tjquinno
suggested changes
Aug 11, 2022
| @@ -152,7 +154,7 @@ Global tags and tags registered with the metric are included in the output retur | |||
|
|
|||
| Global tags MUST NOT be added to the `MetricID` objects. Global tags must be included in list of tags when metrics are exported. | |||
|
|
|||
| NOTE: In application servers with multiple applications deployed, there is one reserved tag name: `_app`, which serves for | |||
| NOTE: In application servers with multiple applications deployed, there is one reserved tag name: `mp_app`, which serves for | |||
There was a problem hiding this comment.
This does not line up with the addition of mp_scope so there is no longer just one reserved tag name. Maybe change this to something like:
NOTE: In application servers with multiple applications deployed, values of the reserved tag mp_app distinguish metrics from...
Channyboy
force-pushed
the
700-mp_scope_mp_app
branch
from
fda4104
to
2418e85
Compare
donbourne
requested changes
Aug 11, 2022
| @@ -154,8 +154,8 @@ Global tags and tags registered with the metric are included in the output retur | |||
|
|
|||
| Global tags MUST NOT be added to the `MetricID` objects. Global tags must be included in list of tags when metrics are exported. | |||
|
|
|||
| NOTE: In application servers with multiple applications deployed, there is one reserved tag name: `_app`, which serves for | |||
| distinguishing metrics from different applications and must not be used for any other purpose. For details, | |||
| NOTE: In application servers with multiple applications deployed, values of the reserved tag `mp_app` distinguishing metrics | |||
There was a problem hiding this comment.
think there's a grammar issue. maybe change to...
values of the reserved tag mp_app distinguish metrics from ...
| It should be possible to override this value by bundling the file `META-INF/microprofile-config.properties` within the application archive | ||
| and setting a custom value for the property `mp.metrics.appName` inside it. | ||
|
|
||
| It is allowed for application servers to choose to not add the _app tag at all. Implementations may differ in how they handle cases where | ||
| It is allowed for application servers to choose to not add the `mp_app`` tag at all. Implementations may differ in how they handle cases where |
There was a problem hiding this comment.
looks like you have too many single quotes surrounding mp_app on this line
tjquinno
approved these changes
Aug 11, 2022
jgallimore
approved these changes
Aug 16, 2022
Channyboy
force-pushed
the
700-mp_scope_mp_app
branch
from
b0209fb
to
2c5fe17
Compare
Channyboy
requested review from
jgallimore,
tjquinno and
donbourne
and removed request for
tjquinno
|
@jgallimore @donbourne @tjquinno Updated with TCK updates |
Channyboy
changed the title
donbourne
requested changes
Aug 17, 2022
| @@ -118,7 +118,7 @@ public void setA() { | |||
| @InSequence(2) | |||
| public void testSharedCounter() { | |||
|
|
|||
| Response resp = given().header("Accept", TEXT_PLAIN).get("/metrics?scope=application"); | |||
| Response resp = given().header("Accept", TEXT_PLAIN).get("/metrics?mp_scope=application"); | |||
There was a problem hiding this comment.
mp_scope should be scope in the /metrics querystring
| @@ -144,7 +144,7 @@ public void setB() { | |||
| @InSequence(4) | |||
| public void testSharedCounterAgain() { | |||
|
|
|||
| Response resp = given().header("Accept", TEXT_PLAIN).get("/metrics?scope=application"); | |||
| Response resp = given().header("Accept", TEXT_PLAIN).get("/metrics?mp_scope=application"); | |||
There was a problem hiding this comment.
mp_scope should be scope in the /metrics querystring
tjquinno
approved these changes
Aug 17, 2022
jgallimore
approved these changes
Aug 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on the following to be merged first.
#707
*^This PR is based off of this as it needs to change text that was added in #707
and
#709
Depends on #709 first before TCKs can be updated.
DRAFT as there still requires changed in TCK.
Fixes #705
(^branch erroneously named as 700 )
REST TCKs in #709 are more explicit in checking Prometheus output in that there are more tests now.
Those tests check for the previous
scope/_scopetag which will now be changed tomp_scope.