Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add metrics Enricher API #1271
Add metrics Enricher API #1271
Changes from 5 commits
468688c
fc0b575
4e641ac
fe2c130
dbe97fe
b297e94
ee0f8fd
21c833f
7b11574
015599a
adf34f1
e6b051e
afa378d
8ea1616
f73d433
208a049
4f91f17
4739df6
85faa2e
b94b328
b3caafa
a22e258
6dab82e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MetricsProcessor
does not seem like an appropriate name for this interface. It is overly general, uses the word "metrics" as a noun instead of an adjective (something that is avoided in OpenTelemetry), and duplicates the name of the othermetric.Processor
from the export package, which is is also a field of theAccumulator
.Can we use a more descriptive name of the function this interface will serve and possibly match broader naming standards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be an interface? It seems like a declared type of
func
would be sufficient here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this method be too specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing the added indirection here of a reference type for the second parameter means that argument will be mutated. This doesn't seem idiomatic of Go and it will likely add an additional allocation of the slice pointer and pose concurrency issues.
Is there any reason this could not accept a slice and return a slice?
Additionally, how are errors expected to be handled here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the processors are only invoked for synchronous instruments, do they not have any value for async instruments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aneurysm9 TBH we haven't started using the async instrument yet, but when implementing this feature I did looked it. Firstly I didn't find a proper place to add this hook then when I explore the code I reckon it is unnecessary to do that since for async instrument the client can provide a callback which already gives ability to achive the same purpose.
e.g. https://github.com/open-telemetry/opentelemetry-go/blob/master/example/basic/main.go#L71 you can re-compose the metric labels using the
context
andcommonLabels
in the callback function. (Please correct me if I'm wrong)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to use this interface for asynchronous instruments, but the applications will be different. There's no distributed context available for this Enricher API, but you still might want to use this API to filter labels before they enter the Accumulator. Enricher is not a good name for a filter, so it feels like we still haven't found the right name for this concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmacd Have you had a chance to look open-telemetry/opentelemetry-specification#1116 (comment)