felt / qgis-plugin

20 stars 5 forks source link

Report FSL conversion warnings to usage API #68

Closed nyalldawson closed 1 month ago

nyalldawson commented 1 month ago

@ChrisLoer Would you mind testing this, and seeing if the reporting of FSL conversion warnings to your API is working well?

ChrisLoer commented 1 month ago

I haven't been able to trigger any warnings using the plugin built from this PR (felt_plugin.2.0.2-alpha.ec1d150.zip) using the data/style from: geology_style (1).zip. I tried adding a scale-based rule because I saw that in the list of error messages, but still no dice:

Screenshot 2024-05-27 at 4 37 43 PM

Can you provide some example data/style I could use to trigger the warning?

nyalldawson commented 1 month ago

@ChrisLoer there's no user facing errors yet, but you should be getting reports via the usage API. Can you check that?

ChrisLoer commented 1 month ago

No I'm not seeing anything come in. Glancing at this code, it looks like it's still using unsupported_layer (which is meant to be "we can't upload the data") vs "unsupported_style" (which is supposed to be "we can't automatically style this"). But even looking for unsupported_layer in our logs, I only see reports coming in from the existing 2.0.1 version of the plugin, and nothing so far today.

Is there a way to see the log warning messages from the QGIS side so I can tell what it's trying to do?

nyalldawson commented 1 month ago

Is there a way to see the log warning messages from the QGIS side so I can tell what it's trying to do?

Can you check the network logger requests? For my testing I'm seeing a submission like this:

curl 'https://felt.com/api/v1/internal/reports' -H 'accept: application/json' -H 'x-qgis-add-to-felt-version: 1.0.0' -H 'authorization: Bearer xxxx' -H 'Content-Type: application/json' -H 'User-Agent: Mozilla/5.0 QGIS/33700/Fedora Linux 40 (KDE Plasma)' --data '{"type": "info", "content": "{\"type\": \"fsl_conversion\", \"warnings\": [{\"object\": \"renderer\", \"renderer\": \"rule_based\", \"cause\": \"scale_based_rule\", \"message\": \"Rule based renderer with scale based rule visibility cannot be converted\", \"level\": \"Error\"}]}"}' --compressed

ChrisLoer commented 1 month ago

Ah yeah, I see it now -- it's coming in with the fsl_conversion type. Sorry I'm badly in need of a nap today. So it's making it to our logs, but not to our upstream analytics because the only two types we automatically record are unsupported_layer and the new unsupported_style using the same item->count syntax that unsupported_layer uses.

Screenshot 2024-05-27 at 5 53 43 PM
ChrisLoer commented 1 month ago

Ah yeah, I see it now -- it's coming in with the fsl_conversion type. Sorry I'm badly in need of a nap today. So it's making it to our logs, but not to our upstream analytics because the only two types we automatically record are unsupported_layer and the new unsupported_style using the same item->count syntax that unsupported_layer uses.

Screenshot 2024-05-27 at 5 53 43 PM
nyalldawson commented 1 month ago

@ChrisLoer so is that something that needs to change in the way the plugin reports this? or in the analytics backend?

ChrisLoer commented 1 month ago

I think the plugin? This was my original request here https://github.com/felt/qgis-plugin-discussion/issues/71 -- I mostly just chose that format because it's what we were already using for unsupported layers and led to a nice analogous hook up to our analytics. As long as it reports the keys/counts in that format, any other data that gets reported will still be accessible to use for a limited time via the logs (we hold them for two weeks).

nyalldawson commented 1 month ago

@ChrisLoer can you try with the latest version?

github-actions[bot] commented 1 month ago

Plugin ready!

A test version of this PR is available for testing here.

(Built from commit d4b857c4c9e9029d79d7cdd5be666ee51ecf23ed)

ChrisLoer commented 1 month ago

This looks like it's working as intended 🎉

Screenshot 2024-05-28 at 3 16 41 PM