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

GDI spec changes + update upstream #15

Merged
4 commits merged into from
Oct 21, 2021
Merged

GDI spec changes + update upstream #15

4 commits merged into from
Oct 21, 2021

Conversation

ghost
Copy link

@ghost ghost commented Oct 19, 2021

  1. Update to newest upstream
  2. Docs update (this is only dev, but FYI @theletterf )
  3. Default values and validation according to the serverless spec, notable changes:
  • SPLUNK_REALM support for traces (and splunk metrics, if emitted by wrappers)
  • SPLUNK_METRICS_ENDPOINT for splunk metrics, if emitted by wrappers
  • default exporter - OTLP/GRPC

@ghost ghost requested review from a team as code owners October 19, 2021 19:44
@ghost ghost self-requested a review as a code owner October 19, 2021 20:31
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Looks good. I had a small change or two and a couple of questions. Thanks!

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
java/scripts/splunk-java-config Show resolved Hide resolved
scripts/splunk-default-config Show resolved Hide resolved
@theletterf
Copy link
Contributor

@kubawach Reviewed. When these changes are going live, we should update the official docs.

Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
# otlp traces exporter
export OTEL_TRACES_EXPORTER="${OTEL_TRACES_EXPORTER:-otlp}"
# otlp-grpc protocol
export OTEL_EXPORTER_OTLP_TRACES_PROTOCOL="${OTEL_EXPORTER_OTLP_TRACES_PROTOCOL:-grpc}"
Copy link

Choose a reason for hiding this comment

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

BTW this does not apply to Python. Python supports otlp_proto_grpc and otlp_proto_http values for OTEL_TRACES_EXPORTER with otlp being an alias for otlp_proto_grpc. If python decides to change the default to OTLP HTTP, this wrapper will automatically switch to OTLP http for python so may be in case of Python, we want to explicitly set OTEL_TRACES_EXPORTER to otlp_proto_grpc.

Not requesting the change in this PR. Just FYI.

if [ -n "$SPLUNK_ACCESS_TOKEN" ] && [ "$OTEL_TRACES_EXPORTER" = "otlp" ]
then
HEADER="X-SF-TOKEN=$SPLUNK_ACCESS_TOKEN,"
export OTEL_EXPORTER_OTLP_HEADERS="$HEADER""${OTEL_EXPORTER_OTLP_HEADERS}"
Copy link

Choose a reason for hiding this comment

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

If the wrappers uses Splunk distros, why do we need to do this? Wouldn't they pick up SPLUNK_ACCESS_TOKEN automatically?


if [ "$OTEL_TRACES_EXPORTER" = "otlp" ] && [ -z "$OTEL_EXPORTER_OTLP_ENDPOINT" ]
then
export OTEL_EXPORTER_OTLP_ENDPOINT="https://ingest.${SPLUNK_REALM}.signalfx.com/v2/trace/otlp"
Copy link

Choose a reason for hiding this comment

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

Is this for OTLP http or grpc?

Copy link

Choose a reason for hiding this comment

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

ENDPOINT - I don't know the grpc endpoint (not published yet), so I used the http (known) one.

dont we need to set the protocol env var then as well? If I understand correctly, we are defaulting to grpc above?

@ghost
Copy link
Author

ghost commented Oct 20, 2021

@owais regarding

@owais
Copy link

owais commented Oct 20, 2021

OTEL_TRACES_EXPORTER - that's a news! How is this possible? According to the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#specify-protocol exporter/protocol values should be as in the PR. Is there a plan for Python to go along with the spec?

Didn't know about that. I think support in Python was added before this got added to the spec. I'll open an issue and add support for protocol env vars.

scripts/splunk-default-config Outdated Show resolved Hide resolved
scripts/splunk-default-config Outdated Show resolved Hide resolved
scripts/splunk-default-config Outdated Show resolved Hide resolved
scripts/splunk-default-config Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Oct 21, 2021

@owais thanks for handling Python variables :) I'l add support for python-specific names in the meantime.

@ghost ghost merged commit c0f7407 into main Oct 21, 2021
@Kielek Kielek deleted the gdi-spec-changes branch January 31, 2022 20:47
This pull request was closed.
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