Fix Collector Metrics Prefix #530

pjanotti · 2020-02-04T03:38:07Z

Fixes the Collector metrics prefix, from "oc_collector" to "otelcol" and make it configurable.
Remove "oc_io" prefix from process metrics.
Remove redundant "otelcol" prefix from observability metrics and "otelsvc_" from label keys.

Description: Fix some basic metric naming issues and make the prefix configurable with default "otelcol".
Link to tracking Issue: Part of #141.
Testing: Manual verification and added a new test to ensure that the new command-line option is working as expected.
Documentation: Further documentation will come with other changes, for now recording the specific changes of this PR:

Default metric prefix updated from "oc_collector" to "otelcol". The prefix can be customized via the --metrics-prefix flag.
Removed the "oc_io" prefix from the process metrics (see https://github.com/open-telemetry/opentelemetry-collector/blob/master/internal/collector/telemetry/process_telemetry.go)
Removed the "otelsvc" from the observability metrics and also the prefix "otelsvc_" from the label keys.

Fixes the Collector metrics prefix, from "oc_collector" to "otelcol" and make it configurable. Remove "oc_io" prefix from process metrics. Remove redundant "otelcol" prefix from observability metrics and "otelsvc_" from label keys.

codecov-io · 2020-02-04T03:53:30Z

Codecov Report

Merging #530 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #530      +/-   ##
==========================================
+ Coverage   76.06%   76.09%   +0.03%     
==========================================
  Files         126      126              
  Lines        7860     7870      +10     
==========================================
+ Hits         5979     5989      +10     
  Misses       1584     1584              
  Partials      297      297

Impacted Files	Coverage Δ
internal/collector/telemetry/process_telemetry.go	`0% <ø> (ø)`	⬆️
observability/observability.go	`50% <ø> (ø)`	⬆️
service/telemetry.go	`78.72% <100%> (+5.75%)`	⬆️

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 47ccdff...8d9ea32. Read the comment docs.

tigrannajaryan · 2020-02-04T13:26:24Z

docs/service-extensions.md

@@ -46,7 +46,7 @@ these sections would look like in the configuration file:

 ```yaml

-# Example of the extensions provided in OTelSvc core. The list below
+# Example of the extensions available OOB with the Collector. The list below


What does OOB stand for?

Out of the box I guess?

The idea was really to to be out of the box, will avoid the acronym.

ibawt · 2020-02-04T13:46:57Z

thanks! Is an empty string a valid prefix? We end up namespacing them anyways, at least in our metric system.

bogdandrutu

It is very unclear why this change is required, explicitly for the exported values like the tag keys.

bogdandrutu · 2020-02-04T18:26:06Z

observability/observability.go


 // TagKeyExporter defines tag key for Exporter.
-var TagKeyExporter, _ = tag.NewKey("otelsvc_exporter")
+var TagKeyExporter, _ = tag.NewKey("exporter")


This is a breaking change.

Correct, trying to get this change more visibility and coordinate release of it.

bogdandrutu · 2020-02-04T18:28:32Z

@rghetia this will break Google usage of the collector, you should review.

bogdandrutu · 2020-02-04T18:42:54Z

observability/observability.go

+	mExporterReceivedSpans      = stats.Int64("exporter/received_spans", "Counts the number of spans received by the exporter", "1")
+	mExporterDroppedSpans       = stats.Int64("exporter/dropped_spans", "Counts the number of spans received by the exporter", "1")
+	mExporterReceivedTimeSeries = stats.Int64("exporter/received_timeseries", "Counts the number of timeseries received by the exporter", "1")
+	mExporterDroppedTimeSeries  = stats.Int64("exporter/dropped_timeseries", "Counts the number of timeseries received by the exporter", "1")


Also this is against the documentation in the opencensus which suggested to prefix metrics with the component name.

By default, all metrics are prefixed with "otelcol" there is no need to repeat it here.

pjanotti · 2020-02-05T01:11:32Z

@bogdandrutu

It is very unclear why this change is required, explicitly for the exported values like the tag keys.

The goal is to get consistent and clear naming for the metrics improving discoverability and readability. Names like "oc_collector_otelsvc_*" can confuse first time users. Since it is a breaking change the time to do it is before it reaches its first official release. That said I know that the Collector is already being used in production so we need to make the breaking change. Reaching folks out to make this clear and perhaps consider some alternatives.

pjanotti · 2020-02-05T01:15:47Z

@ibawt

Is an empty string a valid prefix?

Yes, I don't find a reason to prevent it.

Changing those will be a breaking change for some. Limiting the scope of this change to only fixing the prefix, metrics rename/reorg will come in later.

pjanotti · 2020-02-07T16:22:42Z

I removed the renaming of metrics and labels and limited this one to just changing the prefix, but since it is a command-line flag one can still use the old one if needed.

rghetia

LGTM.

This package is going to allow controlled transition/rename of metrics. The initial implementation just prepares the way to remove the calls to the observability package. In order to be able to rename and change the metrics generated by the Collector without breaking current usage the obsreport package is being introduced. After this is merged a command-line flag can be added to control the generation of the different sets of metrics - initially with the default as legacy only. Link to tracking Issue: Needed for #141, see also the comments of PR #530 Testing: Besides the added tests I have a series of follow up PRs ready, I used these other PRs to manually validate the changes. Documentation: Please refer to doc.go in the new package.

…y#530) Bumps [boto3](https://github.com/boto/boto3) from 1.17.105 to 1.17.106. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.17.105...1.17.106) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Fix Collector Metrics Prefix

8d9ea32

Fixes the Collector metrics prefix, from "oc_collector" to "otelcol" and make it configurable. Remove "oc_io" prefix from process metrics. Remove redundant "otelcol" prefix from observability metrics and "otelsvc_" from label keys.

pjanotti requested review from bogdandrutu, flands, owais, rghetia, songy23 and tigrannajaryan as code owners February 4, 2020 03:38

owais approved these changes Feb 4, 2020

View reviewed changes

tigrannajaryan approved these changes Feb 4, 2020

View reviewed changes

bogdandrutu reviewed Feb 4, 2020

View reviewed changes

Remove typo

3995768

Rollback metric and label renaming

1dce4c1

Changing those will be a breaking change for some. Limiting the scope of this change to only fixing the prefix, metrics rename/reorg will come in later.

rghetia approved these changes Feb 7, 2020

View reviewed changes

pjanotti merged commit d512bdf into open-telemetry:master Feb 7, 2020

pjanotti deleted the fix-metric-namespace branch February 7, 2020 18:39

pjanotti mentioned this pull request Feb 12, 2020

Add the Observability Report Package #544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Collector Metrics Prefix #530

Fix Collector Metrics Prefix #530

pjanotti commented Feb 4, 2020

codecov-io commented Feb 4, 2020

tigrannajaryan Feb 4, 2020

owais Feb 4, 2020

pjanotti Feb 5, 2020

ibawt commented Feb 4, 2020

bogdandrutu left a comment

bogdandrutu Feb 4, 2020

pjanotti Feb 5, 2020

bogdandrutu commented Feb 4, 2020

bogdandrutu Feb 4, 2020

pjanotti Feb 5, 2020

pjanotti commented Feb 5, 2020

pjanotti commented Feb 5, 2020

pjanotti commented Feb 7, 2020

rghetia left a comment

Fix Collector Metrics Prefix #530

Fix Collector Metrics Prefix #530

Conversation

pjanotti commented Feb 4, 2020

codecov-io commented Feb 4, 2020

Codecov Report

tigrannajaryan Feb 4, 2020

Choose a reason for hiding this comment

owais Feb 4, 2020

Choose a reason for hiding this comment

pjanotti Feb 5, 2020

Choose a reason for hiding this comment

ibawt commented Feb 4, 2020

bogdandrutu left a comment

Choose a reason for hiding this comment

bogdandrutu Feb 4, 2020

Choose a reason for hiding this comment

pjanotti Feb 5, 2020

Choose a reason for hiding this comment

bogdandrutu commented Feb 4, 2020

bogdandrutu Feb 4, 2020

Choose a reason for hiding this comment

pjanotti Feb 5, 2020

Choose a reason for hiding this comment

pjanotti commented Feb 5, 2020

pjanotti commented Feb 5, 2020

pjanotti commented Feb 7, 2020

rghetia left a comment

Choose a reason for hiding this comment