Skip to content
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

Update to latest otel-java snapshot #1057

Merged
merged 4 commits into from
Aug 26, 2020
Merged

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Aug 19, 2020

No description provided.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Welcome to tower bridge, we have a great view of the Thames from here. Let's keep these engines running :)

@@ -37,6 +37,11 @@ public void add(double delta, Labels labels) {
agentDoubleCounter.add(delta, LabelBridging.toAgent(labels));
}

@Override
public void add(double v) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know how the coverage of these tests is but if it's possible to add that'd be nice. When adding these bridges I needed them to be (mostly) sure my bridging is sound. Though admittedly these seem simpler than a lot of the tracing / context related stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not aware of any metric-related tests in our repo. Including these bridges

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unable to understand in 2 minutes how to better do that. If this is blocker, I will have to think about it tomorrow, with fresh head

Copy link
Member

Choose a reason for hiding this comment

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

No problem to open issue(s) for anything unresolved in this PR, better to merge it sooner to resolve #1102

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then feel free to approve and merge if build is green :)

Copy link
Member

Choose a reason for hiding this comment

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

I created #1105 for this

@iNikem
Copy link
Contributor Author

iNikem commented Aug 26, 2020

I had to add a bunch of dependencies on context-propagation artifact to force gradle to take the required time-based snapshot. Without it, some unknown version was used locally.

@iNikem iNikem merged commit bc98955 into open-telemetry:master Aug 26, 2020
@iNikem iNikem deleted the java-snapshot branch August 26, 2020 19:03
wyTrivail pushed a commit to wyTrivail/opentelemetry-java-instrumentation that referenced this pull request Aug 26, 2020
* Update to latest otel-java snapshot

* Upgrade to working otel-java snapshot

* Update to latest otel-java snapshot

* Update to latest otel-java snapshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants