Don't set params when send_params is false #771
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was no test to make sure the params are actually set. This patch adds a regression test to make sure this isn't broken through future work.
Currently, the send_params configuration option is checked in appsignal_plug, in Appsignal.Plug.set_params/2-3. Since parameters are now also added from appsignal_phoenix, and because there's already a distinction between sample data and parameters in Appsignal.Span, Appsignal.Span.set_sample_data/3-4 now checks the send_params configuration if the passed key equals "params". The implementation in appsignal_plug can be removed when depending on the upcoming version of this library.
luismiramirez
approved these changes
May 26, 2022
tombruijn
approved these changes
May 27, 2022
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| bump: "patch" | |||
| type: "add" | |||
There was a problem hiding this comment.
Suggested change
| type: "add" | |
| type: "fix" |
Is it not a bug fix?
There was a problem hiding this comment.
No. This simply moves setting the params from the Plug integration (the only place params are set right now) to the main Elixir one. We’re doing this so we don’t have to implement it for the LiveView params again.
jeffkreeftmeijer
added a commit
that referenced
this pull request
May 27, 2022
* Add missing test to set parameter values There was no test to make sure the params are actually set. This patch adds a regression test to make sure this isn't broken through future work. * Don't set params when send_params is false Currently, the send_params configuration option is checked in appsignal_plug, in Appsignal.Plug.set_params/2-3. Since parameters are now also added from appsignal_phoenix, and because there's already a distinction between sample data and parameters in Appsignal.Span, Appsignal.Span.set_sample_data/3-4 now checks the send_params configuration if the passed key equals "params". The implementation in appsignal_plug can be removed when depending on the upcoming version of this library.
jeffkreeftmeijer
added a commit
that referenced
this pull request
May 27, 2022
* Add missing test to set parameter values There was no test to make sure the params are actually set. This patch adds a regression test to make sure this isn't broken through future work. * Don't set params when send_params is false Currently, the send_params configuration option is checked in appsignal_plug, in Appsignal.Plug.set_params/2-3. Since parameters are now also added from appsignal_phoenix, and because there's already a distinction between sample data and parameters in Appsignal.Span, Appsignal.Span.set_sample_data/3-4 now checks the send_params configuration if the passed key equals "params". The implementation in appsignal_plug can be removed when depending on the upcoming version of this library.
|
✔️ All good! |
jeffkreeftmeijer
added a commit
to appsignal/appsignal-elixir-plug
that referenced
this pull request
May 30, 2022
Since appsignal 2.2.13 (which includes appsignal/appsignal-elixir#771 and appsignal/appsignal-elixir#772), the app's configuration is checked whenever parameters or session data is added to the current span. This patch updates the appsignal version dependency to 2.2.13 or higher, and removes the duplicate checks from Appsignal.Plug.
jeffkreeftmeijer
added a commit
to appsignal/appsignal-elixir-plug
that referenced
this pull request
May 30, 2022
Since appsignal 2.2.13 (which includes appsignal/appsignal-elixir#771 and appsignal/appsignal-elixir#772), the app's configuration is checked whenever parameters or session data is added to the current span. This patch updates the appsignal version dependency to 2.2.13 or higher, and removes the duplicate checks from Appsignal.Plug.
jeffkreeftmeijer
added a commit
to appsignal/appsignal-elixir-plug
that referenced
this pull request
Jun 1, 2022
* Use Span.to_map to test Plug.set_conn_data/2 The current Elixir integration version automatically handles the skip parameter and skip session data options, meaning that logic can be removed from the Plug integration. To make sure everything keeps working, this patch adds two tests that ensure the params and session data is properly set right now, and after the removal. * Remove config checks for params and session data Since appsignal 2.2.13 (which includes appsignal/appsignal-elixir#771 and appsignal/appsignal-elixir#772), the app's configuration is checked whenever parameters or session data is added to the current span. This patch updates the appsignal version dependency to 2.2.13 or higher, and removes the duplicate checks from Appsignal.Plug.
jeffkreeftmeijer
added a commit
to appsignal/appsignal-elixir-plug
that referenced
this pull request
Jun 1, 2022
* Use Span.to_map to test Plug.set_conn_data/2 The current Elixir integration version automatically handles the skip parameter and skip session data options, meaning that logic can be removed from the Plug integration. To make sure everything keeps working, this patch adds two tests that ensure the params and session data is properly set right now, and after the removal. * Remove config checks for params and session data Since appsignal 2.2.13 (which includes appsignal/appsignal-elixir#771 and appsignal/appsignal-elixir#772), the app's configuration is checked whenever parameters or session data is added to the current span. This patch updates the appsignal version dependency to 2.2.13 or higher, and removes the duplicate checks from Appsignal.Plug.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the send_params configuration option is checked in
appsignal_plug, in Appsignal.Plug.set_params/2-3.
Since parameters are now also added from appsignal_phoenix, and because
there's already a distinction between sample data and parameters in
Appsignal.Span, Appsignal.Span.set_sample_data/3-4 now checks the
send_params configuration if the passed key equals "params".
The implementation in appsignal_plug can be removed when depending on
the upcoming version of this library.