Skip to content

Conversation

@miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Sep 10, 2025

Description

While working on https://github.com/authzed/spicedb/pull/2557/files#diff-0d9c5ed01c8a90734943ed1c028bda7f63b67ff60c564a042f69222e892ca962R41 noticed that this opencensus library was deprecated in 2023.
Also DatastoreMetricsOptionLegacyPrometheus was marked as deprecated in February of this year.

So, killing two 🐦 with one 🪨 .

Testing

TODO test locally. Spin up Spanner and verify that metrics still flow

References

@github-actions github-actions bot added area/datastore Affects the storage system area/dependencies Affects dependencies labels Sep 10, 2025
@github-actions github-actions bot added the area/cli Affects the command line label Sep 10, 2025
@codecov
Copy link

codecov bot commented Sep 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.72%. Comparing base (dbe5d5e) to head (fd6188c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2558      +/-   ##
==========================================
+ Coverage   77.70%   77.72%   +0.03%     
==========================================
  Files         439      439              
  Lines       53526    53506      -20     
==========================================
- Hits        41586    41584       -2     
+ Misses       9351     9336      -15     
+ Partials     2589     2586       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@miparnisari miparnisari marked this pull request as ready for review September 10, 2025 23:49
@miparnisari miparnisari requested a review from a team as a code owner September 10, 2025 23:49
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

See comments

flagSet.StringVar(&opts.SpannerEmulatorHost, flagName("datastore-spanner-emulator-host"), "", "URI of spanner emulator instance used for development and testing (e.g. localhost:9010)")
flagSet.Uint64Var(&opts.SpannerMinSessions, flagName("datastore-spanner-min-sessions"), 100, "minimum number of sessions across all Spanner gRPC connections the client can have at a given time")
flagSet.Uint64Var(&opts.SpannerMaxSessions, flagName("datastore-spanner-max-sessions"), 400, "maximum number of sessions across all Spanner gRPC connections the client can have at a given time")
flagSet.StringVar(&opts.SpannerDatastoreMetricsOption, flagName("datastore-spanner-metrics"), "otel", `configure the metrics that are emitted by the Spanner datastore ("none", "native", "otel", "deprecated-prometheus")`)
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone is currently using this flag, is it a breaking change to remove it for them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a reason not to merge, but I want to make sure I understand that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would. We should hide the flag and emit a warning if used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to clarify: are y'all saying that i should keep deprecated-prometheus as an option here, and then if they pass that value, i say you are using a deprecated flag; will default to otel metrics?

Copy link
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 keep it for a few releases logging a warning and using otel instead. Then we can finally fully remove it.

@miparnisari
Copy link
Contributor Author

I just noticed that #2329 is also open.

@miparnisari miparnisari marked this pull request as draft September 11, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli Affects the command line area/datastore Affects the storage system area/dependencies Affects dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants