feat: allow users to opt out of internal SDK metrics by LukeWinikates · Pull Request #143 · wavefrontHQ/wavefront-sdk-go

LukeWinikates · 2023-07-24T22:31:06Z

See #35

senders/client_factory.go

internal/noop_registry.go

internal/real_metric_registry.go

internal/registry.go

internal/noop_registry.go

internal/real_metric_registry.go

senders/integration_test.go

LukeWinikates

@suprajanarasimhan what do you think about moving a few of these files into a sub-package to make the organization more obvious:

registry, real_metric_registry, and noop_registry could all move from internal to internal/sdkmetrics or something like that.

suprajanarasimhan · 2023-08-10T00:18:17Z

@suprajanarasimhan what do you think about moving a few of these files into a sub-package to make the organization more obvious:

registry, real_metric_registry, and noop_registry could all move from internal to internal/sdkmetrics or something like that.

I like it because this would really highlight the bigger picture purpose of these files. Do you mind if I try that move? I've never dealt with sub-packages before and I think it would be a good learning experience. (I don't have time to do it today, but I could revisit this tomorrow morning).

Update: I did this. I also moved one more file from package internal to package sdkmetrics: metrics.go.

suprajanarasimhan · 2023-08-10T00:21:57Z

Thanks a lot for the first round of comments, @LukeWinikates! I think it exposed me to some helpful practices and patterns.

internal/lines.go

internal/sdkmetrics/real_metric_registry.go

senders/test_server_test.go

- create a no-op registry when not sending internal metrics - simplify repetitive pattern with instrumenting line formatter -> handler with instrumentation

- make types unexported where possible - introduce test helper function - introduce internal 'sdkmetrics' package - reorganize files

- backfill unit tests for Send* methods on *wavefrontSender - change LineHandler fields to interfaces - fix a bug in trySendWith - rename internalMetricFamily to realSuccessTracker - fix some style spots, some doc strings - add a Flush() method to the internal sdk metric registry, and use it in e2e tests - reformat some long method signatures - add a happy-path test for realRegistry

suprajanarasimhan reviewed Aug 7, 2023

View reviewed changes

senders/client_factory.go Outdated Show resolved Hide resolved

suprajanarasimhan reviewed Aug 7, 2023

View reviewed changes

internal/noop_registry.go Outdated Show resolved Hide resolved

suprajanarasimhan reviewed Aug 7, 2023

View reviewed changes

internal/real_metric_registry.go Outdated Show resolved Hide resolved

suprajanarasimhan reviewed Aug 7, 2023

View reviewed changes

internal/registry.go Show resolved Hide resolved

suprajanarasimhan force-pushed the internal-metrics-optional branch from 1c4ac36 to 62d97a9 Compare August 8, 2023 00:06

suprajanarasimhan marked this pull request as ready for review August 8, 2023 00:10

This comment was marked as outdated.

Sign in to view