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

Exemplar: revert wrong implementation. #1067

Merged
merged 4 commits into from
Mar 18, 2019
Merged

Exemplar: revert wrong implementation. #1067

merged 4 commits into from
Mar 18, 2019

Conversation

songy23
Copy link
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 rakyll and a team as code owners March 14, 2019 21:36
bogdandrutu
bogdandrutu reviewed Mar 15, 2019
View reviewed changes
Copy link
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
Contributor

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
Contributor Author

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
@@ -45,10 +45,10 @@ type ctxKey struct{}
var mapCtxKey = ctxKey{}

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

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
Contributor Author

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
Contributor

rghetia commented Mar 15, 2019

Will this be a breaking change?

@songy23
Copy link
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
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

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

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
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

@bogdandrutu bogdandrutu bogdandrutu left review comments

@rghetia rghetia rghetia approved these changes

@rakyll rakyll Awaiting requested review from rakyll

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