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

Add metrics (writes and error counts, timing) for consumer mode #37

Merged
merged 5 commits into from
Dec 22, 2020

Conversation

56quarters
Copy link
Contributor

Fixes #36

Copy link
Collaborator

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Nice one @56quarters that looks already pretty close. See my comments in line.

I think it would be great if we could cover the metrics as part of the integration test through https://github.com/prometheus/client_golang/tree/master/prometheus/testutil

pkg/metrics/consume.go Outdated Show resolved Hide resolved
pkg/metrics/consume.go Outdated Show resolved Hide resolved
pkg/metrics/consume.go Outdated Show resolved Hide resolved
pkg/app/consume.go Outdated Show resolved Hide resolved
@56quarters
Copy link
Contributor Author

I'll take a look at testing metrics as part of the integration tests.

* Use single metrics struct for producer and consumer
* Move ownership of metrics to App (singleton instance)
* Add tenant ID to all metrics emitted
* Assorted cleanup
@56quarters 56quarters force-pushed the consumer-metrics branch 3 times, most recently from b81752b to b06d66a Compare December 16, 2020 14:56
Copy link
Collaborator

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

I think it's almost there. See my comments relating the Pulsar, GoCollector and ProcessCollector metrics

pkg/app/app.go Outdated Show resolved Hide resolved
integration/consume_integration_test.go Outdated Show resolved Hide resolved
integration/produce_integration_test.go Outdated Show resolved Hide resolved
56quarters and others added 2 commits December 21, 2020 12:01
* Use default registry by default since other libraries do (pulsar client)
  and only use a fresh registry for our integration tests.
Copy link
Collaborator

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the metrics 👍

@56quarters 56quarters merged commit aaf694b into master Dec 22, 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.

Add observability to consume mode
2 participants