Skip to content

chore(sentry): Add environment config option#1890

Merged
olksdr merged 1 commit intomasterfrom
chore/add-env-to-sentry-relay-config
Mar 1, 2023
Merged

chore(sentry): Add environment config option#1890
olksdr merged 1 commit intomasterfrom
chore/add-env-to-sentry-relay-config

Conversation

@olksdr
Copy link
Contributor

@olksdr olksdr commented Feb 28, 2023

This adds the environment configuration option, which also earlier could be set with env variable SENTRY_ENVIRONMENT to make this a bit more explicit, which environment must be reported to Sentry.

fixes #996

#skip-changelog

@olksdr olksdr requested a review from a team February 28, 2023 11:00
@olksdr olksdr self-assigned this Feb 28, 2023
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Two questions, but they are not blockers.

pub enabled: bool,

/// Sets the environment for this service.
pub environment: Option<Cow<'static, str>>,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a Cow? Is there a use case where we generate SentryConfig with a static string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's basically because sentry-rust has this type for environment in ClientOptions and I did not want to mess with it.

Do you think we should change to something else? What would you propose?

Copy link
Member

Choose a reason for hiding this comment

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

OK, that makes sense, all good then. I would have expected a String here and wondered where the Cow came from. I guess with Cow you could even deserialize without owning, although we don't do that for configs AFAIK.

in_app_include: vec!["relay"],
release: Some(RELEASE.into()),
attach_stacktrace: config.enable_backtraces,
environment: sentry.environment.clone(),
Copy link
Member

Choose a reason for hiding this comment

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

Probably more a question for the SDK, but what happens when both SENTRY_ENVIRONMENT and environment are set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will behave like sentry-rust SDK behaves, and according to this part the env will be used when there is nothing set in the config options.

@olksdr olksdr merged commit 15ed5df into master Mar 1, 2023
@olksdr olksdr deleted the chore/add-env-to-sentry-relay-config branch March 1, 2023 10:26
jan-auer added a commit that referenced this pull request Mar 2, 2023
* master:
  doc(py): Add changelog entries (#1900)
  fix(build): Run check when PR is ready for review (#1899)
  chore(project_local): Allow to follow symlinks for projects configs (#1891)
  ref(project): Skip serializing default fields (#1887)
  chore(build): Run changelog check for draft PRs (#1897)
  chore(sentry): Add environment config option (#1890)
  feat(scrubbing): Scrub `span.data.http.query` with default scrubbers (#1889)
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.

Add sentry environment to sentry config section

2 participants