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

Continue on error by default #6

Merged
merged 1 commit into from Mar 23, 2020
Merged

Conversation

liamawhite
Copy link
Contributor

@liamawhite liamawhite commented Mar 22, 2020

Erroring when able to continue is brittle and was causing the following HELP mismatch on the same metric name to break metric collection merge for us.

# HELP process_start_time_seconds Start time of the process since unix epoch.

# HELP process_start_time_seconds Start time of the process since unix epoch in seconds.

When trying to get merged result from the exporter:

An error has occurred while serving metrics:

collected metric process_start_time_seconds label:<name:"cluster" value:"tcc-staging" > counter:<value:1.58467339842e+09 >  has help "Start time of the process since unix epoch." but should have "Start time of the process since unix epoch in seconds."

Signed-off-by: Liam White liam@tetrate.io

Signed-off-by: Liam White <liam@tetrate.io>
@liamawhite liamawhite changed the title continue on error by default Continue on error by default Mar 22, 2020
@odeke-em
Copy link
Member

Hey @liamawhite, thank you for the PR but I'd like to know how that error came about?

@liamawhite
Copy link
Contributor Author

This came from using the Prometheus exporter in the Open Telemetry Collector. These were metrics from two different receivers: a prometheus scraper and an opencensus agent.

Those metrics are produced by the respective JVMs from an instance of SkyWalking and an instance of Zipkin. The metrics have the same name but different help fields which causes an error in the Prometheus library, which I presume isn't sure how to merge them.

Copy link
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Gotcha, thank you @liamawhite for the explanation and for the PR! LGTM and I'll make a release shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants