- 🇳🇱Netherlands thomasdegraaff
Patch attached.
A 'Incident process ratio' setting has been added to the radioactivity field formatter. This setting is a number between 0 and 1 and defaults to 1. If it is set to 0.2 then one in the five incidents is posted to the backend. To accomplish this the incident process ratio is passed to the triggers.js where it is used to determine if a incident should be posted to the backend or not.
The last submitted patch, 4: radioactivity-sample_percentage-3032540-4.patch, failed testing. View results →
- Status changed to Needs work
almost 2 years ago 9:10pm 23 February 2023 - 🇳🇱Netherlands sutharsan
The Math.random() function returns a floating-point, pseudo-random number in the range [ 0, 1) that is, from 0 (inclusive) up to but not including 1. So I believe the comparison
Math.random() <= …
is not correct.The readme document should be extended with a description of the new setting.
What is the rationale of having the new setting per field instance? What is the advantage of having this per field instance? I see possible problems when for example comparing values.
- 🇳🇱Netherlands thomasdegraaff
Thanks for the feedback @Stuharsan. I'll fix the issues you mentioned.
The setting should be site wide indeed. And the emitted value should be divided by the incident process ratio so the values submitted with different incident process ratios are comparable. I'll provide a patch later on.
- 🇳🇱Netherlands thomasdegraaff
Fixes broken link to admin settings page in previous patch, and removes debug console.log in javascript.
- Status changed to Needs review
over 1 year ago 8:49am 6 April 2023 The last submitted patch, 8: radioactivity-sample_percentage-3032540-8.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.The last submitted patch, 9: radioactivity-sample_percentage-3032540-9.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- Status changed to Needs work
over 1 year ago 8:57pm 20 April 2023 - 🇳🇱Netherlands sutharsan
Thanks for working on this issue!
-
+++ b/README.md @@ -119,6 +119,14 @@ value is treated and how it behaves over time. +The emitted energy for a processed incident is divided by the incident process ratio to make the total emitted energy for a certain amount of incidents independent of the incident process ratio setting. This means the incident process ratio setting will not affect the average amount of energy a incident will emit.
I don't understand this.
1. Explain why lowering the value is good on high traffic sites. 2. Describe if it is ok to modify the value on an existing site.
I think you mean multiply instead of divide (not implemented?). But personally I would not bother explaining it.
-
+++ b/src/Form/SettingsForm.php @@ -0,0 +1,82 @@ + '#description' => $this->t('The ratio of incidents to process.') . t('Lower the value to reduce backend server load on high traffic sites.') . '<br/><br/>' . $this->requirementDescription() . '<br/>' . $this->t('For example: 1 or 0.5 or 0.005 etc.') . '<br/><br/>' . t('The emitted energy for a processed incident is divided by the incident process ratio to make the total emitted energy independent of the incident process ratio setting.'), + '#default_value' => $this->config('radioactivity.settings')->get('incident_process_ratio') ?? 1,
concatenating t function results will only work correct in left-to-right languages and not in right-to-left ones. Keep string on together. However
separated strings may be concatenated as strings (= r-t-l safe)Only use $this->t() not t() in classes.
-
+++ b/src/Form/SettingsForm.php @@ -0,0 +1,82 @@ + // Clear cache to make sure the javascript is not cached.
Use short array syntax.
The reference to javascript is confusing to me. The cache contains the field formatter (which in turn contains some JS).
-
+++ b/src/Form/SettingsForm.php @@ -0,0 +1,82 @@ + Cache::invalidateTags(array('radioactivity_incident_process_ratio'));
Use short array syntax!
-
- 🇳🇱Netherlands thomasdegraaff
@Sutharsan Thx for the feedback.
To answer your questions:
1. I'll use an example to explain it. Let's suppose an incident emits 10 energy. Let's suppose there have been 10 incidents. When the incident process ratio is 1, then these 10 incidents will emit 100 energy in total.
Now the incident process ratio is set to 0.5.
There have been 10 incidents again. Now statistically half of the 10 incidents will be processed, so that is 5 incidents. When each incident then emits 10 / 0.5 = 20 energy, then statistically the total energy emitted will be 5 x 20 = 100 energy in total.
This way statistically 10 incidents will store the same amount of energy notwithstanding the incident process ratio that is set.
The reason I explain this in the readme is to prevent users to change the emitted energy to do this correction themselves. Like as when someone sets the incident process ratio from 1 to 0.1 then this person might change the setting for emitted energy from 10 to 1 to not mess up the stats.
2. Thanks for the explanation of the rationale of not concatenating translated strings. I'll fix these issues.
3. I had an issue when testing the incident process ratio functionality that the actual process ratio would not change for a test page after changing the incident process ratio setting. Only after clearing the cache reloading the page would trigger incidents with the configured process ratio setting. The incident process ratio is passed to the javascript via drupalSettings and this seems to be cached so clearing the cache when the setting is changed seemed the right way to fix this.
4. I'll fix this.
- 🇳🇱Netherlands sutharsan
-
+++ b/README.md @@ -119,6 +119,14 @@ value is treated and how it behaves over time. +The ratio of incidents to process. For example: 1 or 0.5 or 0.005 etc. Lower the value to reduce backend server load on high traffic sites.
I would add an example: "For example with a process ration value of 0.2, every fifth page hit (on average) will trigger an incident. The emitted energy per incident is the configured value multiplied by 5."
-
+++ b/src/Plugin/Field/FieldFormatter/RadioactivityReferenceEmitter.php @@ -125,6 +186,9 @@ class RadioactivityReferenceEmitter extends RadioactivityReferenceFormatterBase + // Set custom cache tag to invalidate on incident process ratio config change.
Comment is longer than 80 chars.
-
- Status changed to Needs review
over 1 year ago 9:43am 26 April 2023 - last update
over 1 year ago 73 pass, 1 fail - last update
over 1 year ago 73 pass, 1 fail The last submitted patch, 16: radioactivity-sample_percentage-3032540-16.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.The last submitted patch, 18: 3032540-18.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- Status changed to Needs work
over 1 year ago 6:35am 10 August 2023 - 🇳🇱Netherlands thomasdegraaff
Patch adds default incident process ratio setting of 1.
- last update
over 1 year ago 73 pass, 1 fail - last update
over 1 year ago 73 pass, 1 fail - 🇳🇱Netherlands thomasdegraaff
I can't get the test to pass, and fail to understand what goes wrong. It's quite hard to debug because there's no error messages showing what goes wrong. Maybe someone more experienced in writing functional tests can help?