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

When a custom segment is used in an API request and browser_archiving_disabled_enforce=1, throw an exception explaining to create the segment first #12255

Closed
mattab opened this issue Nov 7, 2017 · 8 comments
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Nov 7, 2017

Use case

As an API user, when I call the API to fetch a segmented report, and use a custom segment (ie. i didn't create it via segment editor but manually constructed the segment parameter in my API request), if the config.ini.php has browser_archiving_disabled_enforce=1, my response will always show "no data", and I don't know why. I need to see clearly in the API response somehow that I need to manually create the segment in the segment editor first, before being able to fetch the data via the API for this segment.

Reproduce

  • browser_archiving_disabled_enforce=1 in config.ini.php
  • call the API with a custom segment (ie. a segment which is not created in the Segment Editor)

Result: always show 0 / no data.

Steps

It's important to fix this issue because it is setup this way for some Piwik service customers.

When browser_archiving_disabled_enforce=1 and the API is used with a &segment= parameter

  1. When the segment exists in the DB and is set as segmented reports are processed in real time (default), then the segment data will never be displayed in standard reports (it will however work in Real time reports). In case the data would never be displayed, we would need to throw an exception:

The Segment is set to "segmented reports are processed in real time (default)" but Piwik has been configured to not process segmented reports in API requests. Therefore to see data for this report in the future, you need to edit your segment and choose "segmented reports are pre-processed (faster, requires cron)". Once you have edited your segment, it will take up to a few hours for your segments data to appear here.

  1. When the segment does not exist in the DB (ie. a "custom" segment), then the data will never be displayed in standard reports (it will however work in real time reports). We would need to throw an exception:

The Segment you specified has not been created yet in the Segment Editor and therefore the report data has not been pre-processed yet. To solve this, you need to go to Piwik and create this segment manually in the Segment editor (alternatively, you can create a new segment pragmatically using the SegmentEditor.add API). Once you have created the segment in the editor (or via API), this error message won't be displayed any more and you will see your segmented reports data within a few hours (once the segment data has been pre-processed).
Please note that you can already test whether your segment will work and match your users by using the Live.getLastVisitsDetails API. When using the Live.getLastVisitsDetails API, you will see in real time which visits and actions were matched by your segment which can help you confirm your &segment= parameter is constructed well and works as expected.

Feedback welcome, the goal is to make it crystal clear to users why there is no data and what to do to get data.

@mattab mattab added Bug For errors / faults / flaws / inconsistencies etc. c: Usability For issues that let users achieve a defined goal more effectively or efficiently. labels Nov 7, 2017
@mattab mattab added this to the 3.2.1 milestone Nov 7, 2017
@sgiehl
Copy link
Member

sgiehl commented Nov 7, 2017

The browser_archiving_disabled_enforce setting currently doesn't overwrite all other settings.
If enable_browser_archiving_triggering is set to 1 or the option in UI was enabled the archiving might be triggered even with browser_archiving_disabled_enforce on.

Shouldn't browser_archiving_disabled_enforce overwrite any other setting? Or should it be renamend to include something with segment, if it is only used for segment archiving

@mattab
Copy link
Member Author

mattab commented Nov 19, 2017

The browser_archiving_disabled_enforce setting currently doesn't overwrite all other settings.
If enable_browser_archiving_triggering is set to 1 or the option in UI was enabled the archiving might be triggered even with browser_archiving_disabled_enforce on.

Shouldn't browser_archiving_disabled_enforce overwrite any other setting? Or should it be renamend to include something with segment, if it is only used for segment archiving

Yes, that's by design. A super user should be able to temporarily enable browser trigger archiving (eg. via UI) to temporarily force triggering some segments even if disabled in the config.

  • We should add a comment to clarify this works this way by design

@mattab mattab modified the milestones: 3.2.1, 3.2.2 Nov 19, 2017
@mattab mattab modified the milestones: 3.5.0, 3.4.1 Mar 26, 2018
@diosmosis diosmosis self-assigned this May 5, 2018
@diosmosis
Copy link
Member

Side note: maybe it would be good to disable/grey-out the segmented reports are processed in real time (default) option if browser archiving is disabled.

@diosmosis
Copy link
Member

@mattab started implementing this and I think it shouldn't be done in core. There are many API methods that don't care about segments or will work w/ segments (and potentially many in future plugins) and there's no way for core to know about all of them. So by throwing an exception we could be creating errors for API methods that should work (eg, listing report metadata). My current solution has a blacklist, but even then plugins will have to remember to add methods to the whitelist, and that shouldn't be their responsibility (in other words, plugin developers shouldn't have to remember to take 'SegmentEditor.apiMethodsThatSupportUnprocessedSegment' for their code to function, as it has nothing to do w/ their actual plugin).

This behavior can be accomplished pretty easily w/ an event on 'Request.dispatch' and no extra code if #12823 is merged. I think it should be done in cloud, where you'll know every API method that's supposed to work w/ this.

@mattab mattab modified the milestones: 3.5.0, 3.5.1 May 8, 2018
@diosmosis
Copy link
Member

@mattab left a comment for you ^, can you take a look when you have a chance?

@mattab
Copy link
Member Author

mattab commented May 21, 2018

Side note: maybe it would be good to disable/grey-out the segmented reports are processed in real time (default) option if browser archiving is disabled.

👍

So by throwing an exception we could be creating errors for API methods that should work (eg, listing report metadata). My current solution has a blacklist, but even then plugins will have to remember to add methods to the whitelist,

blacklist/whitelist solutions (or an event) are definitely not suitable. the only solution is to detect for sure that a segment is being used as part of an archive request (ie. that actively tries to trigger an archive) and only then throw an error. In your example of listing report metadata, the archiver code shouldn't be triggered (the codepath of creating a new archive wouldn't be created), and therefore the error would not be thrown.

I realise the issue description wrongly says "When the segment does not exist in the DB (ie. a "custom" segment), then the data will never be displayed in standard reports" when I really meant something like "When an archive is attempted to be created/read, and the segment does not exist in the DB, then...."

So hopefully there is a way that we can only throw this error in APIs, when the archiving was tried but failed due to the browser_archiving_disabled_enforce=1 setting?

@diosmosis
Copy link
Member

Managed to get something working, thanks for the bit about archiving 👍 Added to #12823

@diosmosis
Copy link
Member

Re disabling, there appears to be a config option for that already, [General] enable_create_realtime_segments.

@diosmosis diosmosis removed this from the 3.5.1 milestone May 24, 2018
@mattab mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Aug 9, 2018
@mattab mattab added this to the 3.6.0 milestone Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

No branches or pull requests

3 participants