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

[BREAKING CHANGE] Enable "new-metrics" by default #1148

Merged
merged 2 commits into from Jun 22, 2020

Conversation

pjanotti
Copy link
Contributor

@pjanotti pjanotti commented Jun 18, 2020

Description:
This change switches the defaults between "new-metrics" and "legacy-metrics".

Now the defaults are:

  • "new-metrics" ON by default
  • "legacy-metrics" OFF by default
  • "add-instance-id" ON by default

Link to tracking Issue: Third item on #1082

Testing: Added test to enforce the "service_instance_id" label ON by default.

Documentation: Updated monitoring.md to reflect that the "new-metrics" are on by default.

This change switches the defaults between "new-metrics" and "legacy-metrics".

Now the defaults are:
- "new-metrics" ON by default
- "legacy-metrics" OFF by default
- "add-instance-id" ON by default
@pjanotti pjanotti self-assigned this Jun 18, 2020
@pjanotti pjanotti changed the title Enable "new-metrics" by default [DO NOT MERGE YET] Enable "new-metrics" by default Jun 18, 2020
@pjanotti
Copy link
Contributor Author

@nilebox @rghetia let's not merge this one until you give the thumbs up.

@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #1148 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
- Coverage   86.86%   86.84%   -0.02%     
==========================================
  Files         201      201              
  Lines       14521    14521              
==========================================
- Hits        12613    12611       -2     
- Misses       1458     1459       +1     
- Partials      450      451       +1     
Impacted Files Coverage Δ
internal/collector/telemetry/telemetry.go 87.93% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 73.52% <0.00%> (-2.95%) ⬇️
service/service.go 52.06% <0.00%> (ø)

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 e3df9f3...b979d9c. Read the comment docs.

Copy link
Member

@nilebox nilebox left a comment

Choose a reason for hiding this comment

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

@pjanotti thanks for waiting for the feedback from Google.
This change should be fine to merge, and we can still override default flags via explicitly enabling "legacy mode" if needed.

We do have custom observability extensions (we use a custom exporter instead of Prometheus there) using the "legacy views" though, e.g.

// ViewReceiverReceivedTimeSeries defines the view for the receiver received timeseries metric.
var ViewReceiverReceivedTimeSeries = &view.View{

As far as I understand, the plan is to delete these legacy views completely, so we will need to migrate our custom extensions to using new views instead. The issue here is that the new metric views are being generated dynamically in private function

func genAllViews() (views []*view.View) {
, so we need to make these views accessible outside of the obsreport package, e.g. we should be able to reference a view for measure
mReceiverAcceptedSpans = stats.Int64(

service/service_test.go Outdated Show resolved Hide resolved
service/service_test.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

@rghetia @mtwo can we get one more +1 from Google? We want to make sure this is not breaking anything for you.

@pjanotti
Copy link
Contributor Author

@nilebox Thanks for reviewing.

As far as I understand, the plan is to delete these legacy views completely, so we will need to migrate our custom extensions to using new views instead. The issue here is that the new metric views are being generated dynamically in private function, so we need to make these views accessible outside of the obsreport package, e.g. we should be able to reference a view for measure

Yes, the plan is to eventually completely remove the legacy views but that should be the very last step. I don't see any problem exposing the obsreport views (I didn't expose them initially since I didn't have a reason to do so).

Copy link
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

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

LGTM.

@pjanotti pjanotti changed the title [DO NOT MERGE YET] Enable "new-metrics" by default Enable "new-metrics" by default Jun 22, 2020
@pjanotti pjanotti merged commit 4bf6afb into open-telemetry:master Jun 22, 2020
@pjanotti pjanotti deleted the switch-default-for-metrics branch June 22, 2020 17:57
@flands flands changed the title Enable "new-metrics" by default [BREAKING CHANGE] Enable "new-metrics" by default Jun 28, 2020
@flands flands added this to the Beta 0.5 milestone Jun 28, 2020
wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
* Enable "new-metrics" by default

This change switches the defaults between "new-metrics" and "legacy-metrics".

Now the defaults are:
- "new-metrics" ON by default
- "legacy-metrics" OFF by default
- "add-instance-id" ON by default

* Use Prometheus parser in tests
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
open-telemetry#1148)

* Bump google.golang.org/grpc from 1.31.1 to 1.32.0 in /exporters/stdout

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.31.1 to 1.32.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.31.1...v1.32.0)

Signed-off-by: dependabot[bot] <support@github.com>

* Auto-fix go.sum changes in dependent modules

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
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.

None yet

5 participants