🐛 [source-google-analytics-data-api] fix issue if metrics or dimensions api response does not have name field #30506
Conversation
octavia-squidington-iii
added
area/connectors
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
brianjlai
changed the title
| metrics_type_map = {h.get("name").replace(":", "_"): h.get("type") for h in r.get("metricHeaders", [{}])} | ||
| dimensions = [h.get("name").replace(":", "_") if "name" in h else None for h in r.get("dimensionHeaders", [{}])] | ||
| metrics = [h.get("name").replace(":", "_") if "name" in h else None for h in r.get("metricHeaders", [{}])] | ||
| metrics_type_map = {h.get("name").replace(":", "_"): h.get("type") for h in r.get("metricHeaders", [{}]) if "name" in h} |
There was a problem hiding this comment.
the old behavior would actually end up with a dictionary that overwrites the None key for any metrics without a name. To me this feels a bit nonsensical so I opted to just skip over metrics missing the name.
As mentioned in the description, I would also be okay if we did the same for the other statements. It's much cleaner, but I'm not sure if there were downstream affects if we deviated from what we considered the earlier working behavior.
|
| Step | Result |
|---|---|
| Connector package install | ✅ |
| Build source-google-analytics-data-api docker image for platform linux/x86_64 | ✅ |
| Unit tests | ✅ |
| Acceptance tests | ✅ |
| Code format checks | ✅ |
| Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml | ✅ |
| Connector version semver check | ✅ |
| Connector version increment check | ✅ |
| QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-google-analytics-data-api test
|
| Step | Result |
|---|---|
| Connector package install | ✅ |
| Build source-google-analytics-data-api docker image for platform linux/x86_64 | ✅ |
| Unit tests | ✅ |
| Acceptance tests | ✅ |
| Code format checks | ✅ |
| Validate airbyte-integrations/connectors/source-google-analytics-data-api/metadata.yaml | ✅ |
| Connector version semver check | ✅ |
| Connector version increment check | ✅ |
| QA checks | ✅ |
☁️ View runs for commit in Dagger Cloud
Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command
airbyte-ci connectors --name=source-google-analytics-data-api test
brianjlai
deleted the
brian/google_analytics_data_api_none_dimension_response
branch
What
Fixes OC issue https://github.com/airbytehq/oncall/issues/2973
How
The previous release didn't run into this issue because we took whatever value either a string or None and stored it into the list or mapping. However, once we started performing the
replace(), it revealed that the API response might have metrics or dimensions that do not have a name field. Given its a P0 at midnight, I wanted to try to retain the prior working behavior as close as possible which would persist Nones into the list. I'm okay if we switch this to only put names if they exist as well.Recommended reading order
x.javay.python🚨 User Impact 🚨
none