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

make all piwik metadata (report/dimension/segment/etc.) available client side as merged/minified asset #6256

Closed
diosmosis opened this issue Sep 20, 2014 · 21 comments
Labels
answered For when a question was asked and we referred to forum or answered it. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@diosmosis
Copy link
Member

Can convert Report/Dimension/Segment classes to JSON structure and store as asset like LESS/JS is now. Can load in client via AJAX by new angularjs service.

@diosmosis diosmosis added c: UI - UX (AngularJS twig less) Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change. labels Sep 21, 2014
@tsteur
Copy link
Member

tsteur commented Sep 22, 2014

Not 100% sure what you mean by that but shouldn't there be an API for this? Probably via Report Metadata? Would also allow other apps such as Piwik Mobile to get this configuration/metadata

@tsteur
Copy link
Member

tsteur commented Sep 22, 2014

At some point later when moving to Angular ideally Piwik also only uses getReportMetadata and getProcessedReport as does Piwik Mobile. Not more. This makes sure to have consistent UI and features across apps and allows other developers to build similar apps. For instance for desktop or their own UI.

@diosmosis
Copy link
Member Author

Using the API means creating AJAX requests every time report metadata is needed when the data is only changed during plugin enabling/disabling. Storing in tmp/assets and serving as merged JS is served will remove many AJAX calls. Other apps can use this data since it will essentially be JSON. Or they can go through the old metadata API. They are not relevant to this ticket.

@diosmosis
Copy link
Member Author

To clarify, the issue isn't about replacing metadata classes w/ JSON files, just to store the data as an asset so it doesn't have to be loaded more than once by the UI.

@tsteur
Copy link
Member

tsteur commented Sep 23, 2014

Ah you mean we do not have to load the PHP Classes more than once? So it is about caching and performance? And it will be merged with all the other JavaScript files?

FYI: In Piwik Mobile I request report metadata only once to get all reports and then getProcessedReport for each individual. In theory we could maybe request report metadata as json and store this as JSON asset? It would be nice to use same API's etc for Piwik and Piwik Mobile so we don't have to maintain two solutions. Report metadata could be enriched for this which I need in Piwik Mobile anyway

@mattab mattab added this to the Short term milestone Sep 25, 2014
@diosmosis
Copy link
Member Author

Sorry @tsteur never got an email about this comment.

It's more about not loading the metadata in the UI more than once, and getting data for Dimensions/Segments and everything else, not just what Piwik provides in the currently provided API methods. Loading the data once in Piwik Mobile is fine, but in a browser it won't work since Piwik doesn't provide cache headers for normal API responses.

we could maybe request report metadata as json and store this as JSON asset?

This is the idea essentially, though I would want the metadata (enriched or new) to mirror the Report/Dimension/Segment classes as closely as possible. And to actually provide Dimension/Segment info. And maybe other components that may be added in the future like Metrics/ProcessedMetrics.

I think the getProcessedReport and other reports should eventually be deprecated; I don't think they will scale in terms of wealth of information. And they mix concerns (report data + report metadata).

EDIT:

And it will be merged with all the other JavaScript files?

I was considering creating a separate file that would just be JSON, like, tmp/assets/metadata.json (w/ the additional tmp/assets/metadata.json.deflate). Ie, it wouldn't be part of the merged JS, just stored & served like it.

@mattab
Copy link
Member

mattab commented Sep 30, 2014

The Metadata can change per-user I think, so if we cache it we likely should cache per-user / per-website

@diosmosis
Copy link
Member Author

Do you remember how it can change per-user? I know it can change per website due to goals (though I want to change that somehow).

@mattab
Copy link
Member

mattab commented Oct 2, 2014

It does not currently change per-user I believe, but it would make sense that in the future it will change per user.

I know it can change per website due to goals (though I want to change that somehow).

do you have some idea how this could be done?

@diosmosis
Copy link
Member Author

do you have some idea how this could be done?

Not at the moment, I haven't actually thought about what would be required. In fact, I don't think I could come up w/ ideas w/o experimenting.

But! I think it should be done. Metadata should not be dynamic.

@mattab
Copy link
Member

mattab commented Oct 2, 2014

Metadata should not be dynamic.

I'm not convinced of that yet... so let's discuss again when this becomes more important.

@diosmosis
Copy link
Member Author

Hard to explain exactly what my issue is, but here's what I see is the problem:

In the Report classes, there's just one for the Goals.get (Reports/Get.php). In the API, there is just one Goals.get method. And yet in report metadata, there are numerous reports that call Goals.get. This is inconsistent.

In this particular case, I'd say idGoal is a secondary dimension for the report. Perhaps the main problem is that Reports aren't allowed to have multiple dimensions?

@mattab
Copy link
Member

mattab commented Oct 2, 2014

Perhaps the main problem is that Reports aren't allowed to have multiple dimensions?

yes maybe allowing reports to have multiple sub-reports would make sense.

Maybe you can create issue for this idea, and close this one?

@diosmosis
Copy link
Member Author

This issue which is about allowing all report metadata to be cached as an asset and loaded in the UI is still useful I think.

@mattab
Copy link
Member

mattab commented Oct 2, 2014

Ok good point, so we could simply make our response dynamically serve 304 headers when content hasn't changed so it's cached in browser but still allows dynamic change of metadata .

@tsteur
Copy link
Member

tsteur commented Oct 3, 2014

BTW: In theory plugins can already limit reports etc for different users etc. I would not be surprised if such plugins are out there

@diosmosis
Copy link
Member Author

@tsteur That seems ok since the asset would have to be regenerated on plugin enable/disable? Anyway, this all sounds like it would be better w/ a POC.

@tsteur
Copy link
Member

tsteur commented Oct 3, 2014

I reckon it means it is about caching/creating compiled asset.js based on site and user. In theory metadata could be also based on any other parameters. Depending what plugins do... So we would have to create something like a hash per generated metadata file and save different compiled asset.js or so (if we ship metadata.js with asset.js). Don't know you are more into this topic ;)

@diosmosis
Copy link
Member Author

Like I said above, I would prefer if metadata weren't able to change based on any parameters, only on what exists in the code (ie, API methods). This would require separating filtering concerns (ie, what user has permissions to what reports) from metadata concerns (what is the name of what report) which I think is only tenable for the web UI (in other words, for code we control). But then, that's what I'm actually interested in ;) Specifically for converting to angularjs.

Anyway, if I ever find the time I'll post POC and maybe the idea will be more fleshed out.

@tsteur
Copy link
Member

tsteur commented Oct 3, 2014

I think I know what you have in mind and can imagine an implementation :) Still, the platform should be not limited in such a case in my opinion. That's actually a big advantage of Piwik. There might be still a solution with changing based on any params. Hope you'll find some time ;)

@mattab mattab modified the milestones: Mid term, Short term Oct 12, 2014
@mattab
Copy link
Member

mattab commented May 5, 2016

This will be done as part of the UI & Client side improvements project, where we slowly move logic from server side controllers towards client side controllers (AngularJS), and improving our HTTP APIs so that clients (eg. browser) can do everything with such APIs. Some work is already done in Piwik 3.0 branch, for more info see this issue #7131

@mattab mattab closed this as completed May 5, 2016
@mattab mattab added the answered For when a question was asked and we referred to forum or answered it. label May 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered For when a question was asked and we referred to forum or answered it. Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

3 participants