Skip to content
This repository was archived by the owner on Jul 31, 2023. It is now read-only.

Exemplar: Use generic interface for attachment values.#1070

Merged
songy23 merged 2 commits intocensus-instrumentation:masterfrom
songy23:exemplar-impl
Mar 20, 2019
Merged

Exemplar: Use generic interface for attachment values.#1070
songy23 merged 2 commits intocensus-instrumentation:masterfrom
songy23:exemplar-impl

Conversation

@songy23
Copy link
Copy Markdown
Contributor

@songy23 songy23 commented Mar 18, 2019

Updates #1058.

Similar to Java PR census-instrumentation/opencensus-java#1779. Though in Go I think we can take advantage of the flexibility on interfaces so I just used interface{} for generic attachment values. WDYT?

@songy23 songy23 requested a review from a team as a code owner March 18, 2019 21:37
Copy link
Copy Markdown
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of nits. Otherwise LGTM.

CountPerBucket []int64 // number of occurrences per bucket
// ExemplarsPerBucket is slice the same length as CountPerBucket containing
// an metricdata for the associated bucket, or nil.
// an emexplar for the associated bucket, or nil.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo. s/emexplar/exemplar/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed.

dd := newDistributionData([]float64{1, 2})
dd.addSample(0.5)
attachments1 := map[string]interface{}{"key1": "value1"}
t1, _ := time.Parse("Mon Jan 2 15:04:05 -0700 MST 2006", "Mon Jan 2 15:04:05 -0700 MST 2006")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: time.Now() should work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Mar 19, 2019

FYI this PR doesn't contain user-visible changes. Will merge by EOD today if there're no other comments/concerns.

@songy23 songy23 merged commit 8a36f74 into census-instrumentation:master Mar 20, 2019
@songy23 songy23 deleted the exemplar-impl branch March 20, 2019 02:41
songy23 added a commit that referenced this pull request Apr 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants