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.

Sign up for GitHub

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

Jump to bottom

Add instrumentationLibraryToLabels method to metrics exporter. #253

Merged
merged 4 commits into from
Jan 14, 2022
Merged

Add instrumentationLibraryToLabels method to metrics exporter. #253

merged 4 commits into from
Jan 14, 2022

Conversation

tbarker25
Copy link
Contributor

@tbarker25 tbarker25 force-pushed the tgb-col-exporter-rewrite branch from 1b20652 to 2e7e376 Compare December 22, 2021 20:34
@jsuereth
Copy link
Contributor

jsuereth commented Dec 22, 2021 •
edited by tbarker25
Loading

Forgot to ask:

Could you also add a configuration parameter in Config.MetricConfig, something like instrumentation_library_labels that defaults to true that users can configure?

Then in the code that maps, only send them if this value is true.

EDIT (tgb): done

@tbarker25 tbarker25 force-pushed the tgb-col-exporter-rewrite branch 2 times, most recently from 8d93e8b to 98a6832 Compare December 23, 2021 15:44
@tbarker25 tbarker25 force-pushed the tgb-col-exporter-rewrite branch 2 times, most recently from b1909b0 to 273f641 Compare December 23, 2021 17:58
@tbarker25 tbarker25 requested a review from kjordy December 23, 2021 18:07
@tbarker25
Copy link
Contributor Author

Forgot to ask:

Could you also add a configuration parameter in Config.MetricConfig, something like instrumentation_library_labels that defaults to true that users can configure?

Then in the code that maps, only send them if this value is true.

Done

@dashpole
Copy link
Contributor

dashpole commented Jan 4, 2022

@jsuereth why does it need to be configurable? The spec says we MUST send these labels on metrics

aabmass
aabmass approved these changes Jan 4, 2022
View reviewed changes
exporter/collector/config.go Show resolved Hide resolved
exporter/collector/metricsexporter.go Outdated Show resolved Hide resolved
@jsuereth
Copy link
Contributor

jsuereth commented Jan 5, 2022

@dashpole

@jsuereth why does it need to be configurable? The spec says we MUST send these labels on metrics

It does. However I think two things are true:

  1. The spec was mostly driven by Tracing exporters (where the cost-benefit analysis vs. tracing may be different)
  2. I suspect we'll likely find that we ALWAYS want instrumentation source, but sometimes want to suppress/ignore instrumentation version. There's a bit of discussion on this around prometheus metric exporter from weyert (can't find the link off hand)

@tbarker25 tbarker25 force-pushed the tgb-col-exporter-rewrite branch 4 times, most recently from db9b8b3 to 25fed6f Compare January 6, 2022 19:28
@tbarker25 tbarker25 force-pushed the tgb-col-exporter-rewrite branch 2 times, most recently from e17bb91 to 0a4c885 Compare January 6, 2022 20:33
@tbarker25 tbarker25 force-pushed the tgb-col-exporter-rewrite branch 2 times, most recently from 051f0e8 to 7dbaafc Compare January 12, 2022 18:27
@tbarker25
Copy link
Contributor Author

Discussed offline:

We should only set instrumentation_library fields for custom metrics (as they can't be added to default metrics).
This means we have two options:

  1. If InstrumentationLibraryLabels is true and metric domain is some custom domain, add those labels. Otherwise don't.
  2. No special handling, just rely on InstrumentationLibraryLabels. GKE and ops agent will need to set it to false.

This CL goes with (1)

jsuereth
jsuereth approved these changes Jan 13, 2022
View reviewed changes
exporter/collector/metrics_test.go Show resolved Hide resolved
exporter/collector/metricsexporter.go Outdated
mes := ilm.Metrics()
for k := 0; k < mes.Len(); k++ {
metric := mes.At(k)
metricLabels := extraResourceLabels

if me.mapper.shouldAddInstrumentationLibraryLabels(metric) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We made this same logic for resources, actually (@dashpole or @aabmass may have the most context on system-metrics being recorded with our APIs).

@dashpole dashpole mentioned this pull request Jan 13, 2022
Merged
@tbarker25 tbarker25 force-pushed the tgb-col-exporter-rewrite branch from 7dbaafc to 6265d0e Compare January 13, 2022 18:54
@tbarker25 tbarker25 force-pushed the tgb-col-exporter-rewrite branch 2 times, most recently from 86ecfde to fc7b038 Compare January 13, 2022 19:01
@tbarker25 tbarker25 force-pushed the tgb-col-exporter-rewrite branch from fc7b038 to bf12754 Compare January 13, 2022 20:43
@tbarker25 tbarker25 merged commit 05f6cee into GoogleCloudPlatform:col-exporter-rewrite Jan 14, 2022
@tbarker25 tbarker25 deleted the tgb-col-exporter-rewrite branch January 14, 2022 17:38
@aabmass aabmass mentioned this pull request Jan 19, 2022
Closed
dashpole added a commit that referenced this pull request Feb 2, 2022
@dashpole @aabmass
@jsuereth @tbarker25 @punya
merge col-exporter-rewrite into main (#295)
7338096 
* Skip all fixture tests (#239)

* Initial structure for new pdata metrics exporter (#238)

* [Metrics Rewrite] add outline with todos for fragmenting work (#240)

* [Metrics Rewrite] attribute to label mapping (#243)

[Metrics Rewrite] attribute to label mapping

* [Metrics Rewrite] support for pdata Sum points (#242)

* [Metrics Rewrite] support for pdata Sum points

* update breaking-changes.md

* use concatentation instead of sprintf

* [Metrics Rewrite] support for pdata Gauge points (#244)

* Add logic to translate metric descriptors and initial flow (#247)

* Fixes from merge.

* Fix tests.

* Clean up test cases, re-disable integration tests.

* Add summary descriptors and label descriptors.

* Fix lint issues.

* Some fixes from review.

* Remove metric import.

* Fixes from review.
- Update default config method
- Simplify some of my lack-of-go expertise.

* Add unit test for metric domains.

* Fixes from review.

* Add breaking changes.

* Fixes from review.

* Update context to be TODO.

* Add support for exponential histograms and exemplars. (#251)

* Add support for exponential histograms and exemplars.

* Fixes from review.

* Fixes from review.

* Fixes from discussion.

* [Metrics Rewrite] implement monitored resource mapping (#252)

* [Metrics Rewrite] implement monitored resource mapping

* review fixes

* [Metrics Rewrite] update breaking-changes.md for monitored resource (#255)

* Add summary mapping to exporter. (#249)

* Add config to call `CreateServiceTimeSeries` (#259)

* Initial implementation of create service time series.

* Add a test case for create service timeseries.

* Add logic to auto-detect project id if not configured.

* Fix from code review

* Fix resource to be one that has retention policy for integration tests.

* Add support for histogram to metrics exporter. (#258)

BUG=210164184

* Re-enable ops-agent self-metric integration test. (#260)

* [Metrics Rewrite] add ExponentialHistogram fixture (#257)

* [Metrics Rewrite] add ExponentialHistogram fixture

* make tests deterministic

* few last changes

* close channel instead of sending a message

* Enable ops agent host metric integration test. (#264)

- There is a bug in upstream agent-metric-processor that sets incorrect units on usage metrics (GoogleCloudPlatform/opentelemetry-operations-collector#72)
- We update the expectations for inculsion of units in CreateTimeSeries
- We disable metric descriptors (for now).  Given the bug in agent-metric-processor, liekly ops-agent will need upstream fix for this first.

* add a feature gate, which defaults to false, for using the re-written exporter (#267)

* Enable Basic integration tests (#266)

* Enable basic counter test.

* Enable delta counter metrics.

- Note: Delta counters are NOW fake-delta (i.e. cumulatives with limited time windows)

* Enable non-monotonic-sum integration test.

* Re-enable summary integration test and fix design issues in summary translation.

- Summary exports percentiles, not quantiles
- Percentiles should include similar double precision in the string.

* Fix recordfixtures script to use featuregate (#270)

* Skip already seen attribute keys when creating LabelDescriptors (#272)

* Reenable GKE metrics agent fixtures (#271)

* Update breaking-changes.md for googlecloudmonitoring/point_count self observability (#277)

* Move logging to use zap-logger and set up self-observability to match collector expectations. (#275)

* Enable metric prefix integraiton tests. (#274)

* enable workloadapis prefix integration test.

* update unknown domain metrics expect.

* Add instrumentationLibraryToLabels method to metrics exporter. (#253)

* Add instrumentationLibraryToLabels method to metrics exporter.

BUG=https://b.corp.google.com/issues/210164355

* Remove custom_metrics_domains behaviour from metrics-exporter.

* Remove dependency on go.opentelemetry.io/collector (#279)

* remove dependency on go.opentelemetry.io/collector

* add ocgrpc metrics to exporters' self-obs metrics (#280)

* Use OC stackdriver exporter to capture self observability metrics as GCM protos (#282)

* Capture ocgrpc self observability metrics (#283)

* make integrationtest not internal (#285)

* Remove internal/ prefix for integrationtest (#288)

* Add batching support to metrics-exporter. (#286)

* Add batching support to metrics-exporter.

* Retry when we fail to write metric descriptors.

* Re-enable workload metrics integration tests (#278)

* update header year for new files (#296)

* Document new CreateMetricDescriptor behavior (#294)

* reenable disabled metrics test (#299)

Co-authored-by: Aaron Abbott <[email protected]>
Co-authored-by: Josh Suereth <[email protected]>
Co-authored-by: Thomas Barker <[email protected]>
Co-authored-by: Punya Biswal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsuereth jsuereth jsuereth approved these changes

@aabmass aabmass aabmass approved these changes

@dashpole dashpole dashpole approved these changes

@kjordy kjordy Awaiting requested review from kjordy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tbarker25 @jsuereth @dashpole @aabmass @kjordy