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

fix: set default max_spans to 1000 #1132

Merged
merged 2 commits into from
Jan 30, 2025
Merged

Conversation

JoshuaMoelans
Copy link
Member

@JoshuaMoelans JoshuaMoelans commented Jan 29, 2025

Fixes #1131

  • set default for sentry_options_new
  • think about other occurrences of SENTRY_SPANS_MAX

@JoshuaMoelans
Copy link
Member Author

We have three of these snippets for transaction/span_start_child and span_finish.
Even if we set the options->max_spans at creating the options object, I'm not sure we can just remove these?

  size_t max_spans = SENTRY_SPANS_MAX;
  SENTRY_WITH_OPTIONS (options) {
      max_spans = options->max_spans;
  }

@JoshuaMoelans JoshuaMoelans marked this pull request as ready for review January 29, 2025 12:34
@supervacuus
Copy link
Collaborator

We have three of these snippets for transaction/span_start_child and span_finish. Even if we set the options->max_spans at creating the options object, I'm not sure we can just remove these?

The sole reason why those exist is that the initial implementer didn't want to scope the remaining code in SENTRY_WITH_OPTIONS. However, they had to declare it outside so that it was accessible, and that naturally led to potential uninitialized use, which led them to initialize it to the most sensible value. 😅

I would leave them in there. They don't hurt anyone. If you want to get rid of them, you will have to put the entire dependent code inside the SENTRY_WITH_OPTIONS, which doesn't add a lot but might visualize the dependency better.

@JoshuaMoelans JoshuaMoelans merged commit 4218313 into master Jan 30, 2025
25 of 26 checks passed
@JoshuaMoelans JoshuaMoelans deleted the joshua/feat/max_spans_default branch January 30, 2025 09:13
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- set default `max_spans` to 1000 ([#1132](https://github.com/getsentry/sentry-native/pull/1132))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against e91529d

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.

Value for max_spans defaults to 0 in sentry_options_new
2 participants