Skip to content

Avoid exposing OpenCensus reference in public APIs#3253

Merged
bogdandrutu merged 1 commit intoopen-telemetry:mainfrom
mxiamxia:hide-oc
May 26, 2021
Merged

Avoid exposing OpenCensus reference in public APIs#3253
bogdandrutu merged 1 commit intoopen-telemetry:mainfrom
mxiamxia:hide-oc

Conversation

@mxiamxia
Copy link
Copy Markdown
Member

@mxiamxia mxiamxia commented May 21, 2021

Description:
Hide OpenCensus references from public APIs in obsreport and obsreporttest packages, there are 3 public APIs in obsreport are using OpenCensus View.

Changes

  1. Moved obsreport.Configure() and AllViews() into internal folder obsreportconfig and made AllViews private, only keep the necessary telemetry APIs in obsreport. It helps to separate obsreport telemetry setup and collector telemetry reporting. Also, it hides the unnecessary public APIs.
  2. Moved the OpenCensus metrics(stats) defines for each components into internal folder obsreportconfig/obsmetrics

TODO

  1. Remove obsreport.ProcessorMetricViewsfrom the package and update it's consumer
  2. update contrib package for above changes.

Checklist,
obsreport

  • doc.go
  • observability.go
  • obsreport.go
    • func Configure(level configtelemetry.Level) (views []*view.View) {}
    • func AllViews() (views []*view.View) {}
  • obsreport_exporter.go
  • obsreport_processor.go
    • func ProcessorMetricViews(configType string, legacyViews []*view.View) []*view.View {}
  • obsreport_receiver.go
  • obsreport_scraper.go
  • obsreport_test.go
  • obsreporttest
    • obsreporttest.go
    • obsreporttest_test.go

Link to tracking Issue:
#2648

Testing:
make all

@mxiamxia mxiamxia requested a review from a team May 21, 2021 18:33
@mxiamxia mxiamxia force-pushed the hide-oc branch 4 times, most recently from ea7fcbb to bf08eb0 Compare May 21, 2021 22:40
Comment on lines +40 to +42
type ObsMetrics struct {
Views []*view.View
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we put this in internal so external users cannot construct it?

Copy link
Copy Markdown
Member Author

@mxiamxia mxiamxia May 25, 2021

Choose a reason for hiding this comment

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

obsreport.ProcessorMetricViews uses it as input in a few components in contrib. We'll need it for contrib changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should remove these calls, we should look into why is this called, and how we can get rid of it. I think they are mainly call into the ProcessorMetricViews to standardize the name, but we have the BuildProcessorCustomMetricName so mostly they can change to just build the name of the metrics with that call.

What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, you're right. I'll make the changes in separate PR since I've made this PR enough now to hide Configure() and AllViews() from external. Updated description as well. PTAL! Thanks

// Configure is used to control the settings that will be used by the obsreport
// package.
func Configure(level configtelemetry.Level) (views []*view.View) {
func Configure(level configtelemetry.Level) *ObsMetrics {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably this should be moved in internal if possible since this API will no longer exists when switching to otel metrics.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, made the changes and updated the PR description.

@mxiamxia mxiamxia force-pushed the hide-oc branch 4 times, most recently from b7f9dcb to 5848f4d Compare May 26, 2021 00:10
@bogdandrutu
Copy link
Copy Markdown
Member

@mxiamxia need a rebase the circleCI is fix on head

@mxiamxia
Copy link
Copy Markdown
Member Author

@mxiamxia need a rebase the circleCI is fix on head

yes, rebased with the latest on head.

@tete17
Copy link
Copy Markdown

tete17 commented Nov 29, 2021

Hey @mxiamxia & @bogdandrutu sorry to hijack this old MR.

I am updating https://github.com/grafana/tempo to make use of the latest version of this component. It seems they made use of this views to expose opencesus metrics from their embedded receivers. grafana/tempo#1142

This PR mentions these api's are hidden in order to expose Opentelemetry metrics but I don't seem to see if that is already possible. They are wondering if we can keep these metrics (accepted_spans, & refused_spans for example) after updating the library or we will need to find another way of doing this.

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.

4 participants