Adding Resource to MetricRecord #1209
Conversation
codeboten
changed the title
| @@ -49,7 +50,7 @@ def test_export(self): | |||
| ) | |||
| labels = {"environment": "staging"} | |||
| aggregator = SumAggregator() | |||
| record = MetricRecord(metric, labels, aggregator) | |||
| record = MetricRecord(metric, labels, aggregator, meter.resource) | |||
There was a problem hiding this comment.
I think it would be good to have a test that tests to see if the resource created in the metric record matches the resource in the meterprovider.
There was a problem hiding this comment.
Updated the ConsoleMetricsExporter to show the resource and updated the test to validate it.
There was a problem hiding this comment.
I think it would be better to remove resource from Meter: codeboten#9
There was a problem hiding this comment.
The spec does specify that a Meter should bind an API with a resource: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics/sdk.md#sdk-terminology, but I guess since we're not making use of the resource yet, it's ok to remove it here for now.
Added it, not sure how I forgot it. |
Remove resource attribute from Meter
| @@ -352,8 +352,7 @@ def __init__( | |||
| instrumentation_info: "InstrumentationInfo", | |||
| ): | |||
| self.instrumentation_info = instrumentation_info | |||
| self.processor = Processor(source.stateful) | |||
| self.resource = source.resource | |||
| self.processor = Processor(source.stateful, source.resource) | |||
There was a problem hiding this comment.
Take a look at this comment. I believe we should actually decouple Tracer from TracerProvider, and equivalently, Meter from MeterProvider by removing the source attribute. We should be passing in configuration through the Provider into the constructor of both Tracer and Meter. resource would be one of those configurations. Not sure if you want to do this as part of this PR however.
There was a problem hiding this comment.
It looks like issue #1181 already tracks this work, I'd rather do this as a follow up to this PR.
* chore: fixing zone from which to fork a new zone * chore: adding tests for creating zone Co-authored-by: Mayur Kale <[email protected]>
Description
Adding the Resource as a parameter to the Batcher (aka Processor see #1203). Not having the resource in the MetricRecord currently causes an issue retrieving resource information in the OTLP exporter.
Fixes #1208
Type of change
Please delete options that are not relevant.
Checklist: