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

[Discussion] Add deployment.environment support #106

Closed
wants to merge 1 commit into from

Conversation

RassK
Copy link
Contributor

@RassK RassK commented Dec 7, 2022

What

Adds deployment.environment resource support via env SIGNALFX_ENV.
In OTel spec this is advised but for SingalFX APM this is required (for grouping and navigating purposes).

https://signalfuse.atlassian.net/browse/APMI-3667

OTel spec suggests OTEL_RESOURCE_ATTRIBUTES but this is not suitable in machine level.

Tests

  • Unit tests added
  • Manual integration test

https://app.signalfx.com/#/apm/traces/fc4620b36ee3c5759bd9f575397e0b1c
image

@RassK RassK requested review from a team as code owners December 7, 2022 12:20
Copy link
Contributor

@pellared pellared left a comment

Choose a reason for hiding this comment

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

None of the distributions support SIGNALFX_ENV. It is legacy. I think we should close this PR.

@RassK
Copy link
Contributor Author

RassK commented Dec 7, 2022

None of the distributions support SIGNALFX_ENV. It is legacy. I think we should close this PR.

I agree, I would have used SPLUNK_ENV but this seems also not used.
OTEL_RESOURCE_ATTRIBUTES unfortunately is not suitable for global scope as more precise scope will overwrite the global env. To encounter it the more precise env needs to contain the content of global env, which means it is going to add multiple change (failure) points.

@theletterf
Copy link
Contributor

All our other distros use resource attributes to deal with this, AFAIK.

@RassK RassK marked this pull request as draft December 7, 2022 14:01
@RassK
Copy link
Contributor Author

RassK commented Dec 7, 2022

All our other distros use resource attributes to deal with this, AFAIK.

I'm not sure how big is the issue for other languages, since world is moving towards containers / pods, where global scope generally equals process scope, 1 service per container / pod, etc.
When you use heavy environments like Windows Server and IIS, then you can expect multiple services per instance. I see even today loads of use cases of multiple IIS websites + windows services for background processing.
This means you would like to set some machine level environment vars to globally describe the environment. Even I would, for eg testing with Splunk APM, it's less overhead to make it automatically pick up machine level variable, so I know from which VM the trace was originated.

@RassK RassK changed the title Add deployment.environment support [Discussion] Add deployment.environment support Dec 7, 2022
@theletterf
Copy link
Contributor

I can see the positive effect there. Wouldn't this have to be added to the GDI specs first though? Could changes be rolled out before that?

@RassK
Copy link
Contributor Author

RassK commented Dec 7, 2022

I can see the positive effect there. Wouldn't this have to be added to the GDI specs first though? Could changes be rolled out before that?

I guess @pellared knowns the process much better.
@pellared any thoughts? - worth pushing or not?

@RassK
Copy link
Contributor Author

RassK commented Dec 7, 2022

Team meeting update: We will not bring any more SINGALFX_* vars.

@Kielek Kielek closed this Dec 7, 2022
@Kielek Kielek reopened this Dec 8, 2022
@RassK
Copy link
Contributor Author

RassK commented Dec 8, 2022

Linking spec issue here > open-telemetry/opentelemetry-specification#3025

@Kielek
Copy link
Contributor

Kielek commented Jan 19, 2023

Closing for now. See internal issue for details.

@Kielek Kielek closed this Jan 19, 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.

4 participants