Add Metric Support #10
Conversation
| desc := r.Descriptor() | ||
| attrs := attributes(service, desc, r.Labels()) | ||
| switch a := r.Aggregator().(type) { | ||
| case aggregator.MinMaxSumCount: |
There was a problem hiding this comment.
is this a complete enumeration of aggregator types at this point, or is this just a starting point?
There was a problem hiding this comment.
Is the only ones currently exported. There are a few other aggregations, but these are the only two used for all (3) current instruments.
| @@ -7,6 +7,8 @@ const ( | |||
| errorCodeAttrKey = "error.code" | |||
| errorMessageAttrKey = "error.message" | |||
|
|
|||
| serviceNameAttrKey = "service.name" | |||
There was a problem hiding this comment.
Just a comment: It's interesting to me that our telemetry sdk adds the service.name attr for spans but not for metrics.
There was a problem hiding this comment.
Yeah, the ServiceName is a first class field in the JSON span structure we send to New Relic, so it became a part of the Span struct in the SDK. The inclusion in the metric is just by knowing it needs to be in the free form attributes. Which makes sense why it is kind of the same way in the SDK. Might be worth refactoring the design in the SDK, but the same could be said of the Metric API I guess. 🤷
| case aggregator.Sum: | ||
| return sum(desc, attrs, a) | ||
| } | ||
| return nil, ErrUnimplementedAgg |
There was a problem hiding this comment.
What happens in otel when this error is returned? Do the metrics get retried next harvest period?
There was a problem hiding this comment.
That's ultimately going to be up to the configured Controller. This error will get rolled up all the way back to the caller of the Export method this exporter implements on the Exporter interface. This method is called by the Controller in the OpenTelemetry Go SDK. The default "push" Controller (currently the only controller) has a configurable error handler function that by default prints the error to STDERR.
MrAlias
force-pushed
the
add-init-metric-support
branch
from
c83d685
to
326b844
Compare
There was a problem hiding this comment.
I have been unable to thoroughly test this end to end with a sample app due to time constraints. However, I believe that you have already addressed all of my concerns. They are:
- Does otel send metrics as a delta or cumulative?
- If an error is returned from
Export, what happens? Will the metrics be retried again with the next cycle? (if so, this would be bad) - Bumping the version of otel in the go.mod file. This was a concern for me because in my attempts to set up a sample app, I kept getting strange compile errors complaining about missing fields. I made sure to use this branch for testing, and then those seemed to have gone away.
Add
Exportmethod to theExporterto implement the"go.opentelemetry.io/otel/sdk/export/metric".Exporterinterface.Add transform logic to the
transformpackage to handle OpenTelemetryRecords and transform them into New RelicMetrics.Update existing end-to-end test validate metric exporting.