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

Add a small artifact that joins TracerSdk and InMemorySpanExporter. #494

Merged

Conversation

carlosalberto
Copy link
Contributor

Used for OpenTelemetry integration testing.

This exposes the mentioned artifact to make testing easier by exposing a TracerSdk along with an InMemorySpanExporter (and we can use this to have some actual examples, similar to the OT Shim testbed cases).

The current design keeps the hierarchy used the the API/SDK (which means we can add a InMemoryMetrics or similar later on).

I had been wrapping the proto Span objects in a friendlier class, but dropped that for now, while we the SpanData issue is solved (so I can either use that or not for this layer).

Also, I'm not totally married with the inmemory name (alternative was sdk-testing or sdk-inmemory), so I'm open to suggestions ;)

Used for OpenTelemetry integration testing.
@bogdandrutu
Copy link
Member

Nice you found the same flaky test I mentioned in #487

Copy link
Member

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

I think we also need some utils to compare the span ids and trace ids. Probably this just updates #485 and does not fix it.

* limitations under the License.
*/

package io.opentelemetry.inmemory.trace;
Copy link
Member

Choose a reason for hiding this comment

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

this can probably be in contrib: io.opentelemetry.contrib.inmemory.trace;

Copy link
Member

Choose a reason for hiding this comment

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

Probably better package io.opentelemetry.sdk.contrib.trace.export;

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Please rebase.

* Put it under contrib.
* Use package io.opentelemetry.sdk.contrib.trace.export
@carlosalberto carlosalberto force-pushed the add_inmemory_artifact branch from 9a91b79 to 680773a Compare August 23, 2019 14:49
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@8c3aabf). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #494   +/-   ##
=========================================
  Coverage          ?   78.98%           
  Complexity        ?      560           
=========================================
  Files             ?       65           
  Lines             ?     1913           
  Branches          ?      188           
=========================================
  Hits              ?     1511           
  Misses            ?      343           
  Partials          ?       59

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c3aabf...680773a. Read the comment docs.

@carlosalberto
Copy link
Contributor Author

@songy23 Done

@bogdandrutu Updated ;)

For the sake of fast iteration, I suggest we merge (if you agree with the new package name), and I will expose the remaining friendly oriented bits (trace/span ids helper functions, SpanData alike wrappers, etc) in another PR.

@@ -0,0 +1,7 @@
description = 'OpenTelemetry InMemory Export'
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to have this in contrib_sdk directory. Contrib is a directory that just depends on the API and contrib_sdk depends on the API + SDK :)

@bogdandrutu
Copy link
Member

@carlosalberto please address the last comment and I will merge.

@carlosalberto
Copy link
Contributor Author

@bogdandrutu Oh, definitely. Sorry, had missed your previous comment ;) Will update in a bit.

@carlosalberto
Copy link
Contributor Author

@bogdandrutu Done ;)

* limitations under the License.
*/

package io.opentelemetry.sdk.contrib.trace.export;
Copy link
Member

Choose a reason for hiding this comment

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

From java9 I think it is not recommended to export different maven artifacts for the same package. Maybe we should rename the package to something more specific. Please fix this in a separate PR.

@bogdandrutu bogdandrutu merged commit 925cf41 into open-telemetry:master Aug 27, 2019
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.

4 participants