-
-
Notifications
You must be signed in to change notification settings - Fork 465
Only set log template if message differs from template #4682
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
Only set log template if message differs from template #4682
Conversation
582ae2d to
588f0f5
Compare
|
did some happy path testing using our samples and those looked good (only adding the template when needed) most notable things I didn't test yet:
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 382.73 ms | 435.41 ms | 52.68 ms |
| ee747ae | 415.92 ms | 470.15 ms | 54.23 ms |
| ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
| ee747ae | 554.98 ms | 611.50 ms | 56.52 ms |
| ee747ae | 358.21 ms | 389.41 ms | 31.20 ms |
| 3699cd5 | 423.60 ms | 495.52 ms | 71.92 ms |
| 85d7417 | 347.21 ms | 394.35 ms | 47.15 ms |
| 7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| ee747ae | 374.71 ms | 455.18 ms | 80.47 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 3699cd5 | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| 85d7417 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| 7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
Previous results on branch: 08-29-only_set_log_template_if_message_differs_from_template
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ed01fec | 419.94 ms | 472.60 ms | 52.67 ms |
| 3e8af17 | 543.45 ms | 633.96 ms | 90.51 ms |
| dc50773 | 382.94 ms | 460.84 ms | 77.90 ms |
| 43b13a8 | 535.13 ms | 613.52 ms | 78.39 ms |
| 880ef1a | 394.84 ms | 450.60 ms | 55.76 ms |
| cf17b6c | 382.60 ms | 439.38 ms | 56.78 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ed01fec | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| 3e8af17 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| dc50773 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| 43b13a8 | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| 880ef1a | 1.58 MiB | 2.10 MiB | 533.44 KiB |
| cf17b6c | 1.58 MiB | 2.10 MiB | 533.43 KiB |
|
Log4j2 lookups work for log message with However this is disabled by default due to log4shell It just shows |
|
Logback encoder also seems to be working with an encoder that replaces every other char with |
|
One more question, would it make sense to have unit tests for the change here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 also +1 for unit tests

📜 Description
Only set template attribute for logging integrations if the formatted message differs from the template.
💡 Motivation and Context
Closes #4675
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps