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 example of creating custom vendor distribution #1621

Merged
merged 8 commits into from
Nov 21, 2020

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Nov 11, 2020

No description provided.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

This is really nice!! Will be great to have people add to this for any additional SPIs that are added.

examples/distro/build.gradle Outdated Show resolved Hide resolved
examples/distro/custom/build.gradle Outdated Show resolved Hide resolved
import org.testcontainers.containers.wait.strategy.Wait;
import org.testcontainers.utility.MountableFile;

abstract class SmokeTest {
Copy link
Member

Choose a reason for hiding this comment

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

can we publish artifact with our SmokeTest and reuse this and OkHttpUtils? (not in this PR)

examples/distro/build.gradle Show resolved Hide resolved
examples/distro/build.gradle Outdated Show resolved Hide resolved
examples/distro/build.gradle Outdated Show resolved Hide resolved
iNikem and others added 4 commits November 12, 2020 10:39
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@pavolloffay
Copy link
Member

a couple of suggestions for this example if we want to advertise it as a supported way of creating custom build:

  • the instrumentation module should live in a separate instrumentation module like in the upstream and it should have unit tests to test the custom fuctionality
  • there should be ByteBuddy plugin configured to generate muzzle references
  • there should be muzzle configured to verify API compatibility for the instrumentation

@iNikem
Copy link
Contributor Author

iNikem commented Nov 13, 2020

  • the instrumentation module should live in a separate instrumentation module like in the upstream

Why is this important?

@iNikem
Copy link
Contributor Author

iNikem commented Nov 13, 2020

Also, @pavolloffay, what do you think about addressing your points in future PRs? At least we cannot do proper muzzle support without #1225, IMO

@pavolloffay
Copy link
Member

Why is this important?

It's important from the testing perspective, I think users would like to unit test instrumentations like we do right now.

Also, @pavolloffay, what do you think about addressing your points in future PRs? At least we cannot do proper muzzle support without #1225, IMO

We should at leat document this in the example readme and point to the issue.

@iNikem
Copy link
Contributor Author

iNikem commented Nov 13, 2020

Unit tests for instrumentations are certainly important. I am wondering why "a separate instrumentation module" is important

@pavolloffay
Copy link
Member

In case you want to have multiple instrumentations and test them in isolation? There might be multiple ways to do it, but following the core repo might be good as well.

@breedx-nr
Copy link

This is really nice!! Will be great to have people add to this for any additional SPIs that are added.

Would you see additions like SpanExporterFactory and MetricExporterFactory examples in a follow-up PR? Should I make an issue to track that?

@trask
Copy link
Member

trask commented Nov 13, 2020

Would you see additions like SpanExporterFactory and MetricExporterFactory examples in a follow-up PR? Should I make an issue to track that?

Yes that would be great 👍

@trask
Copy link
Member

trask commented Nov 13, 2020

I am wondering why "a separate instrumentation module" is important

I think we'll want this once we have muzzle added to the example, since muzzle needs each instrumentation in its own module, so makes sense to me to break it out now, but also happy to merge this PR as-is so that others can start contributing to this effort

@iNikem
Copy link
Contributor Author

iNikem commented Nov 16, 2020

@pavolloffay example of unit-testing should come in a follow-up, as it depends on #1644

examples/distro/README.md Outdated Show resolved Hide resolved
examples/distro/README.md Outdated Show resolved Hide resolved
examples/distro/smoke-tests/src/test/resources/logback.xml Outdated Show resolved Hide resolved
Comment on lines +31 to +35

Response response = client.newCall(request).execute();
System.out.println(response.headers().toString());

Collection<ExportTraceServiceRequest> traces = waitForTraces();
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding // given // when // then comments?

@malafeev
Copy link
Contributor

it could be a challenge for users (for me as well :) to translate gradle scripts to maven.

@trask
Copy link
Member

trask commented Nov 19, 2020

it could be a challenge for users (for me as well :) to translate gradle scripts to maven.

I highly recommend using gradle for custom vendor distributions, at least until we have a brave contributor who documents the equivalent setup for maven. (and I ❤️ maven)

@pavolloffay
Copy link
Member

I am also not sure how our gradle plugins (muzzle, bytecode instrumentation) would work in maven. It wouldn't probably.

@iNikem
Copy link
Contributor Author

iNikem commented Nov 21, 2020

I propose to merge this as a "good enough for now" and to continue improving these examples in the follow-ups

@trask trask merged commit 3185aba into open-telemetry:master Nov 21, 2020
@trask
Copy link
Member

trask commented Nov 21, 2020

agree, this is awesome 👍

@iNikem iNikem deleted the vendor-example branch November 22, 2020 18:25
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.

None yet

7 participants