Skip to content
This repository was archived by the owner on Nov 7, 2022. It is now read-only.

Add basic telemetry to collector#284

Merged
pjanotti merged 3 commits into
masterfrom
add-collector-telemetry
Dec 28, 2018
Merged

Add basic telemetry to collector#284
pjanotti merged 3 commits into
masterfrom
add-collector-telemetry

Conversation

@pjanotti

Copy link
Copy Markdown

This change adds a Prometheus endpoint using OC to provide metrics
to the collector. There are settings to control the level and port
in which the metrics are exposed. The main metrics added for now
are in the queued exporters.

This change adds a Prometheus endpoint using OC to provide metrics
to the collector. There are settings to control the level and port
in which the metrics are exposed. The main metrics added for now
are in the queued exporters.
@pjanotti pjanotti self-assigned this Dec 27, 2018
Comment thread README.md
<-signalsChannel
logger.Info("Starting shutdown...")

// TODO: orderly shutdown: first receivers, then flushing pipelines giving

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

at this point, shouldn't you also gracefully shutdown the http server that is serving metrics?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(shutdown would ideally include waiting the http server to exit before exiting the process)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

To gracefully shutdown here we will need to start having a timeout and so on, just calling Close is a rude shutdown (ie.: underlying listener is closed). This is desirable but we didn't do it yet for the pipes sending data to the backend. I think that adding it now for the handler of Prometheus metrics is an overkill (right now the worst that can happen is a GET to be cut in the middle and Prometheus losing one last scrape of data, but that was already in a race).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense. In the long run when more sophisticated graceful shutdown is desired, you could consider returning a "closeFn" from initTelemetry that is appended to existing list.

Comment thread internal/collector/processor/processor.go Outdated
Comment thread internal/collector/processor/processor.go Outdated
Comment thread internal/collector/processor/processor.go
@pjanotti pjanotti merged commit 8e51c02 into master Dec 28, 2018
@pjanotti pjanotti deleted the add-collector-telemetry branch December 28, 2018 16:17
fivesheep pushed a commit to fivesheep/opencensus-service that referenced this pull request Jun 12, 2019
* Add basic telemetry to collector

This change adds a Prometheus endpoint using OC to provide metrics
to the collector. There are settings to control the level and port
in which the metrics are exposed. The main metrics added for now
are in the queued exporters.

* Check for view registration errors

* PR Feedback
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants