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

Improved Google Analytics integration #11484

Merged
merged 21 commits into from
Nov 5, 2024
Merged

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Nov 4, 2024

Pull Request Description

  • Enhanced Google Analytics API.
  • Now published as a type with static methods not a module.
  • Bump version and add Admin API.
  • Moved the reading logic to Java from Enso.
  • Add dependency on Standard Table allowing report to be built into a Java Table directly.
  • New Google_Credential.new method.
    image
  • Ability to list accounts for a credential (Google_Analytics.list_accounts).
    image
  • Ability to list properties (either for an account or for all) (Google_Analytics.list_properties).
    image
  • Simple object structure of Google_Analytics_Account, Google_Analytics_Property and Google_Analytics_Field with some helper methods.
  • Widget for account, property and credentials.
    image
    image
    image
  • Widget for dimensions and metrics with defaults and then reading from Admin API.
    image
    image
  • Added widget for start_date and end_date on Google_Analytics.read.
  • Bug fix for parse with auto type by reordering to allow numeric dates to be parsed.
  • ToDo: better exception handling.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

Comment on lines 25 to 35
## PRIVATE
to_text : Text
to_text self = case self of
Google_Analytics_Field.Dimension _ -> self.apiName + " (Dimension)"
Google_Analytics_Field.Metric _ -> self.apiName + " (Metric)"

## PRIVATE
to_display_text : Text
to_display_text self = case self of
Google_Analytics_Field.Dimension _ -> "GA Dimension { " + self.apiName + " (" + self.category + ")}"
Google_Analytics_Field.Metric _ -> "GA Metric { " + self.apiName + " (" + self.category + ")}"
Copy link
Member

Choose a reason for hiding this comment

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

What is the reasoning why to_text becomes foo (Dimension) and to_display_text becomes GA Dimension { foo (category) }?

tbh I'm not sure what are our 'guidelines' for what each of these representations shall look like. I know that to_display_text was created in the past to limit the size of text representations of large data structures (i.e. ensuring that a 1M vector can still be displayed and not overwhelm the visualization protocol). So overall to_display_text was meant to be the 'shorter' one.

But I guess over time they also grew that to_display_text is the one 'more' human readable.

I wanted to understand why the particular choice here. Not at all questioning it's correctness - I have absolutely no idea what the text representations should be here - that's why I'm asking.

Comment on lines +250 to +251
## Have to do date parsing first to allow for pure numeric formats.
date_parsers = [self.make_date_time_parser, self.make_date_parser, self.make_time_of_day_parser]
Copy link
Member

Choose a reason for hiding this comment

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

The comment is appreciated, it would not be at all apparent why/if the ordering is significant. Thanks.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

The widgets look awesome!

Got some minor comments/questions, but looks good overall. I would appreciate some pointers explaining some unexpected stuff (like where the "MOCK" account is coming from).


I was a bit on the fence about the main 'read' logic being moved into Java. On one hand, it felt good to have it in Enso because it is a good example of how powerful the language can be - that you can create a whole integration with a Java library purely from Enso, using the polyglot imports. It is certainly possible to do all of this in Enso, although possibly with a bit more hoops here and there. And for the longer term, I think it's important - to be able to convince users that Enso can be used also for more 'low level' library integrations. But for now, I do think it being in Java will probably be quicker to extend and easier to maintain, given the superior tooling. So I'm not saying we should change anything, just wanted to a little bit of sadness that we are not preferring to use our own language, even if it's possible (but in this particular context the decision does make sense).

@somebody1234
Copy link
Contributor

@jdunkerley seems like you've unintentionally checked in (obsolete) files under app/gui2

@jdunkerley jdunkerley added the CI: Keep up to date Automatically update this PR to the latest develop. label Nov 4, 2024
@jdunkerley jdunkerley removed the CI: Keep up to date Automatically update this PR to the latest develop. label Nov 4, 2024
@jdunkerley jdunkerley added CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge labels Nov 5, 2024
@mergify mergify bot merged commit c5734a8 into develop Nov 5, 2024
43 of 44 checks passed
@mergify mergify bot deleted the wip/jd/google-analytics branch November 5, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants