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.

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

Update spec to reflect decision on context-root #108

Closed
wants to merge 1 commit into from

Conversation

mikecroft
Copy link
Member

fixes #65

fixes eclipse#65

Signed-off-by: Mike Croft <backtotheoldhouse@gmail.com>
@mikecroft
Copy link
Member Author

Can someone make sure what I've written clearly reflects the decision from #65 please?

@donbourne @astefanutti @raymondlam

@donbourne
Copy link
Member

Hey Mike, in the Tuesday call, we decided to defer the whole context-root in the name change to 1.1. The reason was that it's a significant change and since we are so close to releasing this we didn't think we'd have time to make that work in our impl and test it properly before putting the spec out.

@mikecroft mikecroft added this to the 1.1 milestone Sep 7, 2017
@pilhuhn pilhuhn removed this from the 1.1 milestone Dec 6, 2017
@@ -126,6 +126,7 @@ Details of this Java API are described in <<app-metrics-api>>.

Application specific metrics are exposed under `/metrics/application`.

The application context root must be prepended to all application metrics to prevent collisions where multiple applications are deployed to the same server. Where the implementation does not support multiple applications on the same server, or an application is deployed to the root context (`/`), it is not necessary to prepend anything to the metric name.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should perhaps use a pseudo-root in cases only one application is provided and not omit that level in the hierarchy to make things uniform (?)

Choose a reason for hiding this comment

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

I agree with @pilhuhn. If only one application is deployed, there should be metrics with both context root prefix and without it. That way it wouldn't break the behavior specified in v1.0.

What happens to the metrics without prefix if another app is deployed would be left unspecified - impls can choose to show the metrics from the app deployed first, last, combine conflicting metrics together or allow user to configure it. I expect that the metrics without a prefix would be only used when there's a single app deployed.

@pilhuhn
Copy link
Contributor

pilhuhn commented Apr 16, 2019

We will not implement it this way, but if at all via labels. See #362

@pilhuhn pilhuhn closed this Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

need a way to avoid metric name collisions for /metrics/application when multiple apps exist
4 participants