This repository has been archived by the owner on Jul 31, 2023. It is now read-only.
Exemplar: Add new record APIs that take exemplar attachments and SpanContext key. #1123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rghetia
reviewed
Apr 22, 2019
stats/record.go
Outdated
| // Measurements will be tagged with the tags in the context mutated by the mutators. | ||
| // RecordWithTags is useful if you want to record with tag mutations but don't want | ||
| // to propagate the mutations in the context. | ||
| func RecordWithTagsAndAttachments(ctx context.Context, mutators []tag.Mutator, attachments metricdata.Attachments, ms ...Measurement) error { |
There was a problem hiding this comment.
Would it make sense to have RecordWithOptions api instead of withAttachment, withTag, WithAttacmentAndTag?
Similar to what we have in gauges (AddInt64Gauge).
There was a problem hiding this comment.
For Attachments that makes sense, though I suggest we still keep the recordWithTags API since it covers most of the use cases and it's easier to use than creating options.
There was a problem hiding this comment.
Updated in 0d5b2d8.
rghetia
pushed a commit
to rghetia/opencensus-go
that referenced
this pull request
Apr 25, 2019
…Context key. (census-instrumentation#1123) * Exemplar: Add SpanContext Attachment key. * Exemplar: Add new record APIs that take exemplar attachments. * Use RetrieveData instead of fake exporter to fix race. * Change map[string]interface to metricdata.Attachments * Use nil instead of empty map. * Update to use options for recording attachments.
rghetia
pushed a commit
that referenced
this pull request
Apr 25, 2019
…Context key. (#1123) * Exemplar: Add SpanContext Attachment key. * Exemplar: Add new record APIs that take exemplar attachments. * Use RetrieveData instead of fake exporter to fix race. * Change map[string]interface to metricdata.Attachments * Use nil instead of empty map. * Update to use options for recording attachments.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Updates #1058.
This is same as #1075 but to the
devbranch.Chatted offline with @rghetia, we think it's better to keep the exemplar APIs as-is since the types of exemplar attachments need to be extended in other packages. Having
map[string]interface{}as exemplar attachment and expose it gives us the most flexibility yet doesn't introduce circular dependencies.