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

prometheus: check return value from CheckpointSet.ForEach #622

Merged
merged 2 commits into from
Apr 7, 2020

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Apr 6, 2020

This PR modifies prometheus.Collect to reflect the change introduced
by #557.

The export{Counter,Histogram,LastValue,Summary} methods now all return
an error instead of calling the error callback directly.
The callback is now only called on the returned error from ForEach.

fixes #563


This change is Reviewable

This PR modifies prometheus.Collect to reflect the change introduced
by open-telemetry#557.

The `export{Counter,Histogram,LastValue,Summary}` methods now all return
an error instead of calling the error callback directly.
The callback is now only called on the returned error from `ForEach`.

fixes open-telemetry#563
Copy link
Member

@paivagustavo paivagustavo left a comment

Choose a reason for hiding this comment

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

LGTM.

For a next step I would love to see us using the go 1.13 error wrapping, I think Josh already mentioned this somewhere.

@oncilla
Copy link
Contributor Author

oncilla commented Apr 6, 2020

@paivagustavo I would be happy to pick that up. (In a separate PR, I guess)
I assume you just want to have additional context.
E.g.

	lv, _, err := lvagg.LastValue()
	if err != nil {
		return err
	}

would turn into something like:

	lv, _, err := lvagg.LastValue()
	if err != nil {
		return fmt.Errorf("error retrieving last value: %w", err)
	}

@paivagustavo
Copy link
Member

Exactly like that @oncilla and indeed in another PR :)

@rghetia rghetia merged commit 6005d01 into open-telemetry:master Apr 7, 2020
oncilla added a commit to oncilla/opentelemetry-go that referenced this pull request Apr 9, 2020
This PR adds error wrapping to the prometheus exporter.
The added context should make it easier to follow the error path.

This is a follow-up to open-telemetry#622
@oncilla oncilla deleted the prom-for-each-error branch April 9, 2020 20:01
jmacd pushed a commit that referenced this pull request Apr 13, 2020
* prometheus: add error wrapping

This PR adds error wrapping to the prometheus exporter.
The added context should make it easier to follow the error path.

This is a follow-up to #622

* feedback
@MrAlias MrAlias added this to the Implement v0.4.0 Specification milestone Jun 3, 2020
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.

Prometheus Collect() should check return value from CheckpointSet ForEach
5 participants