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

Exemplar: revert wrong implementation.#1067

Merged
songy23 merged 4 commits intocensus-instrumentation:masterfrom
songy23:exemplar
Mar 18, 2019
Merged

Exemplar: revert wrong implementation.#1067
songy23 merged 4 commits intocensus-instrumentation:masterfrom
songy23:exemplar

Conversation

@songy23
Copy link
Copy Markdown
Contributor

@songy23 songy23 commented Mar 14, 2019

Updates #1058.

Java counterpart: census-instrumentation/opencensus-java#1791. The first step on refactoring exemplar APIs. More PRs will follow.

@songy23 songy23 requested review from bogdandrutu and rghetia March 14, 2019 21:36
@songy23 songy23 requested review from a team and rakyll as code owners March 14, 2019 21:36
bogdandrutu
bogdandrutu reviewed Mar 15, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Happy to review more PRs to fix this.

trace/exemplar.go Outdated

func init() {
exemplar.RegisterAttachmentExtractor(attachSpanContext)
metricdata.RegisterAttachmentExtractor(attachSpanContext)
Copy link
Copy Markdown
Contributor

@bogdandrutu bogdandrutu Mar 15, 2019

Choose a reason for hiding this comment

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

We need to cleanup these things as well probably. Don't really understand why we need this.

Copy link
Copy Markdown
Contributor Author

@songy23 songy23 Mar 15, 2019

Choose a reason for hiding this comment

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

Agree trace or tag shouldn't depend on exemplar. I removed AttachmentExtractor for now.

tag/context.go Outdated

func init() {
exemplar.RegisterAttachmentExtractor(extractTagsAttachments)
metricdata.RegisterAttachmentExtractor(extractTagsAttachments)
Copy link
Copy Markdown
Contributor

@bogdandrutu bogdandrutu Mar 15, 2019

Choose a reason for hiding this comment

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

This is a wrong usage of exemplars, tags are not "exemplars"

Copy link
Copy Markdown
Contributor Author

@songy23 songy23 Mar 15, 2019

Choose a reason for hiding this comment

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

Removed.

@songy23 songy23 force-pushed the exemplar branch 3 times, most recently from 60c842a to 8ab21ab Compare March 15, 2019 18:45
@rghetia
Copy link
Copy Markdown
Contributor

rghetia commented Mar 15, 2019

Will this be a breaking change?

@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Mar 15, 2019

This is a breaking change to those who import "go.opencensus.io/exemplar". Though I don't believe any user would be using the previous exemplar APIs since no backend supports it before.

For trace, tag, stats and metrics packages, I don't think there's any breaking change.

@songy23 songy23 changed the title Exemplar: move to metricdata. Exemplar: revert wrong implementation. Mar 15, 2019
@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Mar 18, 2019

This PR is only doing cleanups. More PRs will follow on refactoring the exemplar APIs.

@songy23 songy23 merged commit 604812a into census-instrumentation:master Mar 18, 2019
@songy23 songy23 deleted the exemplar branch March 18, 2019 21:33
@aviaoh
Copy link
Copy Markdown

aviaoh commented Mar 19, 2019

@songy23 @rghetia
This PR breaks go get -v go.opencensus.io/exemplar with the following error:

Fetching https://go.opencensus.io/exemplar?go-get=1
Parsing meta tags from https://go.opencensus.io/exemplar?go-get=1 (status code 200)
get "go.opencensus.io/exemplar": found meta tag get.metaImport{Prefix:"go.opencensus.io", VCS:"git", RepoRoot:"https://github.com/census-instrumentation/opencensus-go"} at https://go.opencensus.io/exemplar?go-get=1
get "go.opencensus.io/exemplar": verifying non-authoritative meta tag
Fetching https://go.opencensus.io?go-get=1
Parsing meta tags from https://go.opencensus.io?go-get=1 (status code 200)
go.opencensus.io (download)
package go.opencensus.io/exemplar: cannot find package "go.opencensus.io/exemplar" in any of:
	/usr/local/go/src/go.opencensus.io/exemplar (from $GOROOT)
	/Users/avia/dev/go/src/go.opencensus.io/exemplar (from $GOPATH)

This also breaks go get github.com/Azure/go-autorest @jhendrixMSFT

@aviaoh aviaoh mentioned this pull request Mar 19, 2019
Merged
@aviaoh
Copy link
Copy Markdown

aviaoh commented Mar 19, 2019

please see suggestion for solution in census-ecosystem/opencensus-go-exporter-ocagent#49

@jhendrixMSFT jhendrixMSFT mentioned this pull request Mar 19, 2019
Closed
@songy23
Copy link
Copy Markdown
Contributor Author

songy23 commented Mar 19, 2019

Sorry for the trouble, I wasn't aware that there're downstream libraries depending on this. #1071 should fix it temporarily, meanwhile please migrate to the new package like census-ecosystem/opencensus-go-exporter-ocagent#49.

@orlangure orlangure mentioned this pull request Mar 20, 2019
Closed
songy23 added a commit that referenced this pull request Apr 3, 2019
@songy23
Exemplar: revert wrong implementation. (#1067)
fe327dd 
* Exemplar: move to metricdata.

* Remove AttachmentExtractor.

* More cleanup.

* Add a TODO for next PR.

(cherry picked from commit 604812a)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rakyll rakyll Awaiting requested review from rakyll

2 more reviewers

@bogdandrutu bogdandrutu bogdandrutu left review comments

@rghetia rghetia rghetia approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

exemplar refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@songy23 @rghetia @aviaoh @bogdandrutu