🌱 Monitoring: use opentelemetry for cron metrics #3503
Conversation
Signed-off-by: Raghav Kaul <[email protected]>
raghavkaul
had a problem deploying
to
integration-test
GitHub Actions
Failure
Signed-off-by: Raghav Kaul <[email protected]>
raghavkaul
temporarily deployed
to
integration-test
GitHub Actions
Inactive
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3503 +/- ##
===========================================
- Coverage 74.30% 59.36% -14.95%
===========================================
Files 187 175 -12
Lines 13378 12556 -822
===========================================
- Hits 9941 7454 -2487
- Misses 2878 4653 +1775
+ Partials 559 449 -110 |
| metric.WithDescription("Measures the retry delay when dealing with secondary rate limits"), | ||
| metric.WithUnit("s")) | ||
| if err != nil { | ||
| return |
There was a problem hiding this comment.
I think there's some unintentional shadowing here since you use c, err := above
| // // GithubTokens tracks the usage/remaining stats per token per resource-type. | ||
| // | ||
| // GithubTokens = view.View{ | ||
| // Name: "GithubTokens", | ||
| // Description: "Token usage/remaining stats for Github API tokens", | ||
| // Measure: RemainingTokens, | ||
| // TagKeys: []tag.Key{TokenIndex, ResourceType}, | ||
| // Aggregation: view.LastValue(), | ||
| // } |
There was a problem hiding this comment.
this comment should probably be removed?
| if err != nil { | ||
| l.Warn(&LogMessage{Text: fmt.Sprintf("tag.New: %v", err)}) | ||
| } | ||
|
|
||
| ctx, err = tag.New(ctx, tag.Upsert(stats.RepoHost, r.CheckRequest.Repo.Host())) | ||
| ctx = context.WithValue(ctx, stats.RepoHost, r.CheckRequest.Repo.Host()) |
There was a problem hiding this comment.
does it make a difference when you pass the value as an attribute, vs in the context? You do both in this PR.
Skimming one migration page, I thought otel didnt support context based attributes?
| defer func() { | ||
| if err := meterProvider.Shutdown(context.Background()); err != nil { | ||
| fmt.Printf("couldn't shutdown meterProvider: %v\n", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
wouldn't this close the meter provider as soon as the function returns?
I think the Go idiom you're looking forward is returning a cancel/close/cleanup function, and then the caller can call/defer that when appropriate.
| "contrib.go.opencensus.io/exporter/stackdriver" | ||
| "contrib.go.opencensus.io/exporter/stackdriver/monitoredresource/gcp" | ||
| "go.opencensus.io/stats/view" | ||
| mexporter "github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric" |
There was a problem hiding this comment.
would gcpexporter provide more context?
| ProjectID: projectID, | ||
| MetricPrefix: prefix, | ||
| MonitoredResource: gcp.Autodetect(), |
There was a problem hiding this comment.
Is this information being lost in the new implementation?
| exporter, err := config.GetMetricExporter() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error during GetMetricExporter: %w", err) | ||
| } | ||
|
|
||
| fmt.Printf("Using %s exporter for metrics\n", exporter) |
There was a problem hiding this comment.
leftover print statement?
| ctx, err := tag.New(ctx, tag.Upsert(stats.CheckName, r.CheckName)) | ||
| err := stats.InitMetrics() | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
is this something that should be setup in an init?
|
Stale pull request message |
What kind of change does this PR introduce?
Use opentelemetry instead of opencensus