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

Adding OT Collector metrics exporter #454

Merged
merged 15 commits into from
Mar 11, 2020

Conversation

hectorhdzg
Copy link
Member

Current implementation use OpenCensus receiver available in Collector, this will need to be updated eventually to use OT receiver when is ready.

Fixes #344

@hectorhdzg hectorhdzg requested a review from a team February 28, 2020 23:55
@codecov-io
Copy link

codecov-io commented Feb 29, 2020

Codecov Report

Merging #454 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #454   +/-   ##
=======================================
  Coverage   88.98%   88.98%           
=======================================
  Files          43       43           
  Lines        2216     2216           
  Branches      248      248           
=======================================
  Hits         1972     1972           
  Misses        173      173           
  Partials       71       71           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5f3a7f...e514c71. Read the comment docs.

@hectorhdzg hectorhdzg added exporters metrics needs reviewers PRs with this label are ready for review and needs people to review to move forward. labels Mar 2, 2020
@hectorhdzg hectorhdzg added this to the Beta milestone Mar 2, 2020
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

I have some concerns with the timestamps, it's not totally clear for me when they are needed and where to get them from. Currently the aggregator saves the last time it was updated, but it doesn't set the time it was reset, and I think it's the start timestamp the opencensus-proto refers to.

I tested the example and works fine, some minimal documentation is missing IMO.

start_timestamp=utils.proto_timestamp_from_time_ns(
metric_record.metric.get_handle(
metric_record.label_set
).last_update_timestamp

Choose a reason for hiding this comment

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

I'm not sure it's the correct value we should insert there. The OpenCensus documentation states this is the time when the value was reset to zero: https://github.com/census-instrumentation/opencensus-proto/blob/be218fb6bd674af7519b1850cdf8410d8cbd48e8/src/opencensus/proto/metrics/v1/metrics.proto#L127, also, is this value meaningful for other instruments than counter?

Anyway, I don't know if we should spend so much time discussing here as this will be changed to use the opentelemetry proto.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like there is not such thing in OpenTelemetry as the time when the metric was reset, not sending anything and leaving Collector to handle it would be a better approach here

@@ -0,0 +1,5 @@
scrape_configs:

Choose a reason for hiding this comment

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

I'd be nice to have some documentation about how to access the Prometheus dashboard.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking it will make sense to have it in #423, including all instructions on how to start Collector in docker, I can add a README as part of this PR as well

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW the URL to access Prometheus is http://localhost:9090/graph

examples/metrics/collector.py Outdated Show resolved Hide resolved
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

I did a second round and it looks good, I also tested it and I was able to see the output on the Prometheus dashboard.

I think the should add a big note on the readme indicating that this is actually using the OpenCensus collector, we should avoid creating confusion.

I'm also fine with the current example for this PR, but it has to be improved later on, for instance, integrating with other examples, writing better documentation, etc.

Addressing comments
Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

The export logic and tests LGTM. Just a few comments, mostly on the docs and examples.

I see test_pymongo_functional is still the only integration test. Is the idea to add a test using the collector docker image later, or is it only for the example?

Committing suggestions

Co-Authored-By: Chris Kleinknecht <[email protected]>
@hectorhdzg
Copy link
Member Author

hectorhdzg commented Mar 9, 2020

@c24t docker tests are definitely there for any E2E scenarios, testing the exporters E2E can be tricky unless the exporter expose some API to verify the data was exported, for the Collector I was thinking it would be easier to test E2E once the OpenTelemetry exporter is available, so basically Python SDK exports to Collector and then Collector exports back to OpenTelemetry, then we validate data flowed correctly and Python exporter code was successful.

#472

Copy link
Member

@c24t c24t 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 addressing my comments!

@hectorhdzg hectorhdzg added PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) and removed needs reviewers PRs with this label are ready for review and needs people to review to move forward. labels Mar 10, 2020
@c24t c24t merged commit dd8521e into open-telemetry:master Mar 11, 2020
@c24t c24t removed the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Mar 11, 2020
toumorokoshi added a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 16, 2020
Total Changelog:

Documentations has been significantly overhauled, including:

* a getting started guide
* examples has been consolidated to an docs/examples folder
* several minor improvements to the examples in each extension's readme.

- Adding Correlation Context API and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding a global configuration module to simplify setting and getting
  globals
  ([open-telemetry#466](open-telemetry#466))
- Rename metric handle to bound metric
  ([open-telemetry#470](open-telemetry#470))
- Moving resources to sdk
  ([open-telemetry#464](open-telemetry#464))
- Implementing propagators to API to use context
  ([open-telemetry#446](open-telemetry#446))
- Implement observer instrument for metrics
  ([open-telemetry#425](open-telemetry#425))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Renaming TraceOptions to TraceFlags
  ([open-telemetry#450](open-telemetry#450))
- Renaming TracerSource to TraceProvider
  ([open-telemetry#441](open-telemetry#441))

- Adding Correlation Context SDK and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding OT Collector metrics exporter
  ([open-telemetry#454](open-telemetry#454))
- Improve validation of attributes
  ([open-telemetry#460](open-telemetry#460))
- Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span()
  (open-telemetry#469)
  ([open-telemetry#469](open-telemetry#469))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Make counter and MinMaxSumCount aggregators thread safe
  ([open-telemetry#439](open-telemetry#439))

- Initial release. Support is included for both trace and metrics.
c24t pushed a commit to c24t/opentelemetry-python that referenced this pull request Mar 16, 2020
Total Changelog:

Documentations has been significantly overhauled, including:

* a getting started guide
* examples has been consolidated to an docs/examples folder
* several minor improvements to the examples in each extension's readme.

- Adding Correlation Context API and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding a global configuration module to simplify setting and getting
  globals
  ([open-telemetry#466](open-telemetry#466))
- Rename metric handle to bound metric
  ([open-telemetry#470](open-telemetry#470))
- Moving resources to sdk
  ([open-telemetry#464](open-telemetry#464))
- Implementing propagators to API to use context
  ([open-telemetry#446](open-telemetry#446))
- Implement observer instrument for metrics
  ([open-telemetry#425](open-telemetry#425))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Renaming TraceOptions to TraceFlags
  ([open-telemetry#450](open-telemetry#450))
- Renaming TracerSource to TraceProvider
  ([open-telemetry#441](open-telemetry#441))

- Adding Correlation Context SDK and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding OT Collector metrics exporter
  ([open-telemetry#454](open-telemetry#454))
- Improve validation of attributes
  ([open-telemetry#460](open-telemetry#460))
- Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span()
  (open-telemetry#469)
  ([open-telemetry#469](open-telemetry#469))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Make counter and MinMaxSumCount aggregators thread safe
  ([open-telemetry#439](open-telemetry#439))

- Initial release. Support is included for both trace and metrics.
@hectorhdzg hectorhdzg deleted the hectorhdzg/collector_metrics branch March 24, 2020 22:34
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* fix(span): rename span recording flag

* Update packages/opentelemetry-types/src/trace/SpanOptions.ts

Co-Authored-By: Mayur Kale <[email protected]>

* fix: linting errors
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.

Add OT collector metrics exporter
4 participants