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

Add new threshold parameters to monitor config #3181

Merged
merged 4 commits into from
Feb 5, 2024

Conversation

lbloder
Copy link
Collaborator

@lbloder lbloder commented Feb 1, 2024

📜 Description

💡 Motivation and Context

Fixes #3131

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed (Add thresholds to Crons documentation Java sentry-docs#9053).
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link
Contributor

github-actions bot commented Feb 1, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against b643e5f

Copy link
Contributor

github-actions bot commented Feb 1, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 418.81 ms 541.78 ms 122.97 ms
Size 1.70 MiB 2.27 MiB 584.64 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c7e2fbc 372.00 ms 461.71 ms 89.71 ms
bc4be3b 360.40 ms 435.04 ms 74.64 ms
4e260b3 388.40 ms 468.98 ms 80.58 ms
7ab32b6 376.73 ms 449.27 ms 72.54 ms
283d83e 416.81 ms 497.22 ms 80.41 ms
e6ffd7b 379.52 ms 449.30 ms 69.78 ms
0404ea3 332.47 ms 401.12 ms 68.66 ms
606074f 385.10 ms 428.24 ms 43.14 ms
c7e2fbc 377.85 ms 426.35 ms 48.50 ms
b172d4e 352.92 ms 446.50 ms 93.58 ms

App size

Revision Plain With Sentry Diff
c7e2fbc 1.72 MiB 2.29 MiB 576.40 KiB
bc4be3b 1.72 MiB 2.29 MiB 576.53 KiB
4e260b3 1.72 MiB 2.27 MiB 554.95 KiB
7ab32b6 1.70 MiB 2.27 MiB 584.63 KiB
283d83e 1.72 MiB 2.29 MiB 577.69 KiB
e6ffd7b 1.72 MiB 2.29 MiB 579.13 KiB
0404ea3 1.72 MiB 2.29 MiB 577.52 KiB
606074f 1.70 MiB 2.27 MiB 582.26 KiB
c7e2fbc 1.72 MiB 2.29 MiB 576.40 KiB
b172d4e 1.72 MiB 2.29 MiB 578.09 KiB

Previous results on branch: feat/threshold_monitor_config

Startup times

Revision Plain With Sentry Diff
19bd609 404.32 ms 493.43 ms 89.11 ms
f134080 392.54 ms 461.30 ms 68.76 ms
26a1b06 372.18 ms 461.00 ms 88.82 ms

App size

Revision Plain With Sentry Diff
19bd609 1.70 MiB 2.27 MiB 584.64 KiB
f134080 1.70 MiB 2.27 MiB 584.64 KiB
26a1b06 1.70 MiB 2.27 MiB 584.64 KiB

@lbloder lbloder changed the title dd new threshold parameters to monitor config Add new threshold parameters to monitor config Feb 1, 2024
@lbloder lbloder marked this pull request as ready for review February 1, 2024 14:50
Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, do we need the new ctor overload? We should follow up with a docs PR.

private @Nullable Map<String, Object> unknown;

public MonitorConfig(final @NotNull MonitorSchedule schedule) {
this.schedule = schedule;
}

public MonitorConfig(
Copy link
Member

Choose a reason for hiding this comment

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

m We don't have a ctor overload for other fields except schedule. Do you think we should add an overload for the new ones?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question, I don't think we really need them.
I'll remove the additional ctor, we can re-add some additional ctors if the need arises.

@adinauer adinauer self-assigned this Feb 5, 2024
@lbloder lbloder merged commit 5e04ee8 into main Feb 5, 2024
26 checks passed
@lbloder lbloder deleted the feat/threshold_monitor_config branch February 5, 2024 16:37
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.

CRONS new monitor config options
2 participants