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.

Sign up for GitHub

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

Jump to bottom

feat(Analytics): Support for Timeseries Aggregated Statistics #3207

Merged
merged 33 commits into from
Sep 15, 2021
Merged

feat(Analytics): Support for Timeseries Aggregated Statistics #3207

merged 33 commits into from
Sep 15, 2021

Conversation

rslanka
Copy link
Contributor

@rslanka rslanka commented Sep 7, 2021

Adds support for doing aggregations over time-series aspect fields, and migrates the existing usage stats implementation on top this feature. The key changes include

  • two new annotations @TimeseriesField and @TimeseriesFieldCollection to the PDL for defining the aggregatable fields
  • necessary elastic index generation logic
  • a generalized query end-point via the new analytics end-point.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

rslanka added 19 commits August 23, 2021 16:51
@rslanka
WIP: temporal stats
271ae84
@rslanka
Minor refactoring and cleanup
7bc57e3
@rslanka
WIP: temporal stats
7995420
@rslanka
Minor refactoring and cleanup
1401ccc
@rslanka
Merge branch 'timeseries_agg_stats' of https://github.com/acryldata/d…
f21137f 
…atahub-fork into timeseries_agg_stats
@rslanka
Minor Fix: Setting field values correctly in TimeseriesAspectTransfor…
2188612 
…mer::transform
@rslanka
1) Added sum aggregation + tests
8730456 
2) Analytics end-point clean up.
@rslanka
Added the timeseries aspect DatasetUsageStatistics
7906da0
@rslanka
1. UsageStats Ingestion porting to timeseries aspects
caaa329 
2. Minor other fixes
@rslanka
Address review comments.
63f74bf
@rslanka
UsageStats readpath integration + equivalent tests
78a0cf7
@rslanka
Revert changes to gradle.properties
e28402e
@rslanka
Fix smoke_test test_gms_usage_fetch
161dcf7
@rslanka
Refactored timeseries stat field extraction logic
afc252b
@rslanka
Addressed CR feedback
2510f43
@rslanka
Merge branch 'master' into timeseries_agg_stats
cda8176
@rslanka
Fix the TimeseriesFieldCollection annotation on DatasetUsageStatistic…
80a1673 
…s aspect
@rslanka
1. Fix UsageStats - rename fieldCounts.fieldName to `fieldCounts.fi…
41d6450 
…eldPath` to match the renamed field in the PDL.
@rslanka
Merge branch 'master' into timeseries_agg_stats
20cef22
dexter-mh-lee
dexter-mh-lee reviewed Sep 8, 2021
View reviewed changes
Copy link
Contributor

@dexter-mh-lee dexter-mh-lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a string grouping and then date grouping, will it work?

In general, we need more docs (Logic overview, API examples, etc.). Maybe add them in the next PR!

metadata-io/src/main/java/com/linkedin/metadata/entity/ebean/EbeanEntityService.java
import static com.linkedin.metadata.entity.ebean.EbeanUtils.parseSystemMetadata;
import static com.linkedin.metadata.entity.ebean.EbeanUtils.toAspectRecord;
import static com.linkedin.metadata.entity.ebean.EbeanUtils.toJsonAspect;
import static com.linkedin.metadata.entity.ebean.EbeanUtils.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we revert this file? Seems like the only change is on the comments. Let's just change that and make it clear what we are changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static imports change in this file is a result of running the linkedin code formatter on this file!

@rslanka
Copy link
Contributor Author

rslanka commented Sep 8, 2021

If we have a string grouping and then date grouping, will it work?
@dexter-mh-lee, Yes, it would work! The only limitation on the grouping buckets is that if a date grouping bucket is specified, it is allowed only on the timestampMillis field because of the way the index is built.

In general, we need more docs (Logic overview, API examples, etc.). Maybe add them in the next PR!
Sure, will ship them in the next PR.

jjoyce0510
jjoyce0510 reviewed Sep 8, 2021
View reviewed changes
entity-registry/src/main/java/com/linkedin/metadata/models/EntitySpecBuilder.java
DEFAULT,
/**
* Skip annotations on aspect record fields, only
* parse entity + aspect annotations.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: might be useful to describe briefly why we need this mode

shirshanka
shirshanka approved these changes Sep 15, 2021
View reviewed changes
Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@shirshanka shirshanka merged commit c418bc8 into datahub-project:master Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dexter-mh-lee dexter-mh-lee dexter-mh-lee left review comments

@jjoyce0510 jjoyce0510 jjoyce0510 left review comments

@shirshanka shirshanka shirshanka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rslanka @dexter-mh-lee @shirshanka @jjoyce0510 @gabe-lyons