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

Always serialize schemas when available #2406

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

Anilm3
Copy link
Contributor

@Anilm3 Anilm3 commented Dec 4, 2023

Description

Currently schemas are only provided to the extension when there's an event. This PR changes the subscriber to always add serialized schemas to meta whenever they're available.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@Anilm3 Anilm3 force-pushed the anilm3/always-serialize-schemas branch from 97aca8b to 2f7d88a Compare December 5, 2023 09:27
@Anilm3 Anilm3 marked this pull request as ready for review December 5, 2023 10:08
@Anilm3 Anilm3 requested a review from a team as a code owner December 5, 2023 10:08

double se_sample_rate = get_global_DD_API_SECURITY_REQUEST_SAMPLE_RATE();
if (se_sample_rate >= MIN_SE_SAMPLE_RATE) {
mpack_write_bool(w, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to add some comments explaining what this line is

@Anilm3 Anilm3 merged commit fecdcfc into master Dec 5, 2023
3 checks passed
@Anilm3 Anilm3 deleted the anilm3/always-serialize-schemas branch December 5, 2023 11:06
@github-actions github-actions bot added this to the 0.95.0 milestone Dec 5, 2023
}
}

if (schema.length() <= max_schema_size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to add a debug line here saying schema key has been discarded because of size.

@pierotibou pierotibou removed the tracing label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants