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

[operator/java] Add instrumentation doc for java #2253

Merged

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Feb 2, 2023

Adds a section in the k8s operator instrumentation docs for Java.

Preview: https://deploy-preview-2253--opentelemetry.netlify.app/docs/k8s-operator/automatic/#java

@TylerHelmuth TylerHelmuth requested review from a team as code owners February 2, 2023 18:20
@TylerHelmuth
Copy link
Member Author

/cc @jack-berg

content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
TylerHelmuth and others added 2 commits February 2, 2023 14:27
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Overall LGTM modulo some passive voice and misspellings

content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
content/en/docs/k8s-operator/automatic.md Outdated Show resolved Hide resolved
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
- baggage
sampler:
type: parentbased_traceidratio
argument: "1"
Copy link
Member

Choose a reason for hiding this comment

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

Can you link to the documentation for these configuration options? I'm not familiar with where this is defined or how it translates to the environment variable based config scheme the otel java agent used.

Copy link
Member Author

Choose a reason for hiding this comment

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

For propagators the valid values are tracecontext;baggage;b3;b3multi;jaeger;xray;ottrace;none (https://github.com/open-telemetry/opentelemetry-operator/blob/1a12ac7bb23f78a39a1b6a7f216faa034342b2f7/apis/v1alpha1/propagators.go#L19). The operator will add them to the OTEL_PROPAGATORS environment variable.

For sampler the valid values are always_on, always_off, traceidratio, parentbased_always_on, parentbased_always_off, parentbased_traceidratio, jaeger_remote, xray (https://github.com/open-telemetry/opentelemetry-operator/blob/main/docs/api.md#instrumentationspecsampler). The operator will add the sampler and its args to OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG.

I agree that this page doesn't discuss those aspects clearly, and neither does the Operator docs. I will get an operator docs PR created to address this so that I have something to link to from this doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this blob required, or is this sorta a best practice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you asking if a propagator and sampler are required or only a best practice? They are best practices.

Copy link
Contributor

@cartermp cartermp Feb 6, 2023

Choose a reason for hiding this comment

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

@jack-berg since @TylerHelmuth is adding this content upstream, are you satisfied with the changes for now? We can file a tracking issue to link to the config options (or pull them in here) separately.

@cartermp
Copy link
Contributor

Since the remaining question was about additional configuration options, which are in-progress here, I'll merge this in for now and file a tracking issue to link to them once they are merged in the upstream operator repository. Alternatively, that information can also be brought into docs here, but I think the sequencing is still likely to involve getting open-telemetry/opentelemetry-operator#1442 merged first. Either way, let's not hold up the party here.

@cartermp cartermp merged commit 208befc into open-telemetry:main Feb 15, 2023
@cartermp
Copy link
Contributor

Filed #2341

@TylerHelmuth TylerHelmuth deleted the java-operator-instrumentation branch February 15, 2023 15:37
@chalin
Copy link
Contributor

chalin commented Feb 15, 2023

@cartermp - this broke the build since the PR was submitted before the CI formatting checks were in place

image

Can someone submit a followup PR? If not, let me know and I'll do it.

@chalin
Copy link
Contributor

chalin commented Feb 15, 2023

Actually, here's a fix:

@chalin
Copy link
Contributor

chalin commented Feb 15, 2023

Fixed!

@chalin
Copy link
Contributor

chalin commented Feb 15, 2023

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

4 participants