search

adobe / helix-run-query

service that executes queries on BigQuery datasets generated by Helix-Logging
Apache License 2.0
6 stars 11 forks source link

Transfer to v3 #1024

Open MarquiseRosier opened 11 months ago

MarquiseRosier commented 11 months ago

A minimal set of data endpoints necessary to run https://data.aem.live/rum-dashboard

Also I updated rum-pageviews to use Events_V4; because the version that uses Events_V3 cannot resolve www.petplace.com to petplace.com

When we use Events_V4 which prepends www and checks for results for a url, www.petplace.com and petplace.com can appear.

@trieloff @langswei @dbrrt

github-actions[bot] commented 11 months ago

This PR will trigger a minor release when merged.

codecov[bot] commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (387b11e) 100.00% compared to head (c39dc76) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1024 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 6 6 Lines 710 710 ========================================= Hits 710 710 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

langswei commented 11 months ago

I believe that not every PR merged into main automatically results in v3 semantic versioning. A conventional commit message such as feat: updated dashboard query must be included as at least one of the commits. Lars can confirm if my understanding is correct.

langswei commented 11 months ago

Please run SQLFluff.

langswei commented 11 months ago

Given the move to _V4 table functions in some queries, I suggest creating a file similar to /src/queries/common-event-v3.sql where the new table functions are documented.

MarquiseRosier commented 11 months ago

@langswei I don't think I'll add a query to record the new table function as it wouldn't really fit into this PR, feel free to document the new table functions if you'd like in another PR though!

Events_V4 was created by @trieloff sometime ago; some queries simply haven't been upgraded to use it.

langswei commented 11 months ago

@langswei I don't think I'll add a query to record the new table function as it wouldn't really fit into this PR, feel free to document the new table functions if you'd like in another PR though!

Events_V4 was created by @trieloff sometime ago; some queries simply haven't been upgraded to use it.

Sounds good, separate PR makes sense, no reason to combine here. My overarching goal is to ensure queries used by data.aem.live and queries used to feed spacecat and eventually DaaS -- especially those related to page view estimates -- are based on common logic so that we maintain consistency. I agree with you this is a separate topic.