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

PR for otelp-metrics #108

Merged
merged 6 commits into from
Feb 23, 2021
Merged

Conversation

bryce-b
Copy link
Member

@bryce-b bryce-b commented Feb 8, 2021

I'm opening this draft PR to get some feedback on the changes I've made to the existing api to support the otelp for metrics.

Here are key notes for these changes:

- Made MeterSdkProvider init mimic the TraceProvider init

        I decided to change the MetricSdkProvider.init method to take shared state objects as parameters,
        since this is how the TraceProvider operates. Alternatively, I could have move the default values
        into the MeterSharedState construction, but that would require making the NoopMetricProcessor &
        NoopMetricExporter public, which I'm not against, but seemed intentionally private.

- replaced MeterRegisteryKey with InstrumentationLibraryInfo

- Injected Resource into the MetricSdk stack
        I followed the same pattern as with the Trace Sdk.

@nachoBonafonte
Copy link
Member

It looks great so far.
About making Noop... public, I cannot imagine a use case for a user of the library to use a NoopMetricProcessor or a NoopMetricExporter. I would only make public what can be useful for the user.

@bryce-b
Copy link
Member Author

bryce-b commented Feb 9, 2021

I cannot imagine a use case for a user of the library to use a NoopMetricProcessor or a NoopMetricExporter

I agree, the issue I ran into was using them as default values for the sharedMeterState's constructor, which would have required them to be public since they would be available in the constructor definition.

@bryce-b
Copy link
Member Author

bryce-b commented Feb 10, 2021

I'm looking for some guidance in the toProtoMetric method in Sources/Exporters/OpenTelemetryProtocol/metric/MetricsAdapter.swift

I'm not seeing a way to set min/max values. The closest proto metric I could find for the summary metrics was a histogram. I was thinking that the explicitBounds might satisfy the "min/max" issue, but I don't think that's quite right. Would exemplars be the means to capture the min max values?

@nachoBonafonte
Copy link
Member

nachoBonafonte commented Feb 11, 2021

Not an expert here, but I would say so. Checking opentelemetry-go code, they have this method and in line 261 they convert to histogram with this. I hope it can help you

@bryce-b bryce-b force-pushed the bryce/otelp-metrics branch 3 times, most recently from 23632f8 to e476aa7 Compare February 16, 2021 22:45
and stubbed MetricsAdapter & OtelpMetricExporter
- Made MeterSdkProvider init mimic the TraceProvider init

        I decided to change the MetricSdkProvider.init method to take shared state objects as parameters,
        since this is how the TraceProvider operates. Alternatively, I could have move the default values
        into the MeterSharedState construction, but that would require making the NoopMetricProcessor &
        NoopMetricExporter public, which I'm not against, but seemed intentionally private.

- replaced MeterRegisteryKey with InstrumentationLibraryInfo

- Injected Resource into the MetricSdk stack
        I followed the same pattern as with the Trace Sdk.
@bryce-b bryce-b marked this pull request as ready for review February 17, 2021 22:54
@bryce-b bryce-b changed the title Draft PR for otelp-metrics PR for otelp-metrics Feb 18, 2021
@nachoBonafonte
Copy link
Member

nachoBonafonte commented Feb 22, 2021

Sorry for the delay in reviewing, i was OOO since last Wednesday.
The code looks really great, and I think that your changes made the usage much nicer and consistent. Just to be picky, I saw some styling issues mainly with multiple line breaks, but can be merged like this. (I use swiftformat for auto fixing style)

@nachoBonafonte
Copy link
Member

Thanks a lot for your contribution.
Let me know if you cannot merge the PR and I will do.

@bryce-b
Copy link
Member Author

bryce-b commented Feb 23, 2021

I'll make sure I use swiftformat in the future! I'm not able to merge however; take it away!

@nachoBonafonte nachoBonafonte merged commit 3161c2b into open-telemetry:main Feb 23, 2021
@bryce-b bryce-b deleted the bryce/otelp-metrics branch March 1, 2021 16:52
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

2 participants