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

GTC-2904: Datamart Endpoint #590

Draft
wants to merge 15 commits into
base: develop
Choose a base branch
from
Draft

Conversation

gtempus
Copy link
Contributor

@gtempus gtempus commented Oct 4, 2024

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Check the commit's or even all commits' message styles matches our requested structure.
  • Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe): This is a proof of concept (PoC). We don't want to merge this PR; only learn from it.

What is the current behavior?

Coarse-grained vs. Fine-grained Services (Endpoints)

API clients must know how to use the fine-grained endpoints (query, etc) to query datasets with tedious SQL. This is great when you are doing ad-hoc work, but once a client settles on and becomes dependent on specific queries, a more coarse-grained endpoint should be created to serve their needs. This Thoughtworks article explains the spirit of what a coarse-grained endpoint should be trying to accomplish.

Issue Number: GTC-2904

What is the new behavior?

This PoC introduces the concept of organized services that are ready for consumption by various clients without needing to know the intricacies of our DB schema. The organization is inspired by Flagship's map navigation:

organize_datamart

Testing

You can hit this endpoint using cURL:

curl --location 'http://gfw-data-api-elb-shared-dev-lb-10091095.us-east-1.elb.amazonaws.com:30616/datamart/analysis/forest_change/tree_cover_change/net_tree_cover_change?iso=BRA&x-api-key=<your_api-key>' \
--data ''

An example response:

{
    "data": {
        "iso": "BRA",
        "adm1": null,
        "adm2": null,
        "stable": 413722809.3,
        "loss": 36141245.77,
        "gain": 8062324.946,
        "disturb": 23421628.86,
        "net": -28078920.83,
        "change": -5.932759761810303,
        "gfw_area__ha": 850036547.481532
    },
    "status": "success"
}

Does this introduce a breaking change?

  • Yes
  • No

Other information

Flagship Integration

The client of this endpoint can be reviewed in this Flagship PR. It demonstrates how much business logic will be removed by embracing a coarse-grained approach.

@gtempus gtempus force-pushed the gtc-2904/datamart_endpoint branch 3 times, most recently from 60f3899 to 27eb115 Compare October 4, 2024 23:03
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 60.00000% with 44 lines in your changes missing coverage. Please review.

Project coverage is 80.79%. Comparing base (2fbb5b9) to head (6a6d58e).

Files with missing lines Patch % Lines
...tamart/analysis/forest_change/tree_cover_change.py 51.11% 44 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #590      +/-   ##
===========================================
- Coverage    81.18%   80.79%   -0.40%     
===========================================
  Files          129      133       +4     
  Lines         5842     5952     +110     
===========================================
+ Hits          4743     4809      +66     
- Misses        1099     1143      +44     
Flag Coverage Δ
unittests 80.79% <60.00%> (-0.40%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


class Gadm(str, Enum):
ISO = "iso"
ADM0 = "adm0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems confusing - adm0 and ISO are the same, right? Should you eliminate one of them (probably ADM0), or add a comment on why you have both of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I was never happy with that, either. I think I remember it having something to do with the existing query params (interface) that made it clunky. I'll see what I can do. Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants