feat(chart-unfurls): Add chart unfurl support for world map #29435
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shruthilayaj
changed the title
Contributor
size-limit report
|
static/app/chartcuterie/discover.tsx
Outdated
| discoverCharts.push({ | ||
| key: ChartType.SLACK_DISCOVER_WORLDMAP, | ||
| getOption: (data: {seriesName: string; stats: {data: EventsGeoData}}) => { | ||
| const countryCodesMap = require('app/data/countryCodesMap'); |
Member
There was a problem hiding this comment.
You can just import this at the top. We don't need to get fancy with webpack import stuff here since this file is just loaded into memory of chartcuterie.
shruthilayaj
changed the title
Comment on lines
187
to
216
| if "worldmap" in display_mode: | ||
| try: | ||
| resp = client.get( | ||
| auth=ApiKey(organization=org, scope_list=["org:read"]), | ||
| user=user, | ||
| path=f"/organizations/{org_slug}/events-geo/", | ||
| params=params, | ||
| ) | ||
| except Exception as exc: | ||
| logger.error( | ||
| "Failed to load events-geo for unfurl: %s", | ||
| str(exc), | ||
| exc_info=True, | ||
| ) | ||
| continue | ||
| else: | ||
| try: | ||
| resp = client.get( | ||
| auth=ApiKey(organization=org, scope_list=["org:read"]), | ||
| user=user, | ||
| path=f"/organizations/{org_slug}/events-stats/", | ||
| params=params, | ||
| ) | ||
| except Exception as exc: | ||
| logger.error( | ||
| "Failed to load events-stats for unfurl: %s", | ||
| str(exc), | ||
| exc_info=True, | ||
| ) | ||
| continue |
Contributor
There was a problem hiding this comment.
Can we simplify this so we're setting the path variable and then calling try/except once?
Suggested change
| if "worldmap" in display_mode: | |
| try: | |
| resp = client.get( | |
| auth=ApiKey(organization=org, scope_list=["org:read"]), | |
| user=user, | |
| path=f"/organizations/{org_slug}/events-geo/", | |
| params=params, | |
| ) | |
| except Exception as exc: | |
| logger.error( | |
| "Failed to load events-geo for unfurl: %s", | |
| str(exc), | |
| exc_info=True, | |
| ) | |
| continue | |
| else: | |
| try: | |
| resp = client.get( | |
| auth=ApiKey(organization=org, scope_list=["org:read"]), | |
| user=user, | |
| path=f"/organizations/{org_slug}/events-stats/", | |
| params=params, | |
| ) | |
| except Exception as exc: | |
| logger.error( | |
| "Failed to load events-stats for unfurl: %s", | |
| str(exc), | |
| exc_info=True, | |
| ) | |
| continue | |
| path = 'events-stats' | |
| if "worldmap" in display_mode: | |
| path = 'events-geo' | |
| try: | |
| resp = client.get( | |
| auth=ApiKey(organization=org, scope_list=["org:read"]), | |
| user=user, | |
| path=f"/organizations/{org_slug}/{path}/", | |
| params=params, | |
| ) | |
| except Exception as exc: | |
| logger.error( | |
| "Failed to load events-geo for unfurl: %s", | |
| str(exc), | |
| exc_info=True, | |
| ) | |
| continue |
Contributor
There was a problem hiding this comment.
Errr, and update the logger.error message
edwardgou-sentry
approved these changes
Oct 29, 2021
Contributor
edwardgou-sentry
left a comment
edwardgou-sentry
left a comment
There was a problem hiding this comment.
Just a small nit on naming but otherwise LGTM
static/app/chartcuterie/discover.tsx
Outdated
| const mapSeries = MapSeries({ | ||
| map: 'sentryWorld', | ||
| name: data.seriesName, | ||
| data: data.stats.data.map(x => ({name: x['geo.country_code'], value: x.count})), |
Contributor
There was a problem hiding this comment.
I think we can probably rename x to something more meaningful
wmak
approved these changes
Oct 29, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding a World Map chart unfurl type since
it's a valid visualization on Discover. This uses
chartcuterie's init config to register the sentryWorld
map to chartcuterie's global echarts object.
Added Do Not Merge because this needs toThis has been deployedbe merged and deployed to production first.
Screenshots of map with and without dat
a