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

[APM] Add AWS and Azure icons for additional services #101901

Merged
merged 3 commits into from
Jun 14, 2021
Merged

[APM] Add AWS and Azure icons for additional services #101901

merged 3 commits into from
Jun 14, 2021

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Jun 10, 2021 •
edited
Loading

Closes #96284

Added icons for AWS and Azure services and added storybook for better visibility.

image

@sorenlouv sorenlouv requested a review from a team as a code owner June 10, 2021 14:15
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 10, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@sorenlouv sorenlouv changed the title [APM] Add DynamoDB icon [APM] Display AWS logo for DynamoDB Jun 10, 2021
cauemarcondes
cauemarcondes approved these changes Jun 10, 2021
View reviewed changes
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@sorenlouv sorenlouv changed the title [APM] Display AWS logo for DynamoDB [APM] Display AWS and Azure icons Jun 14, 2021
@sorenlouv sorenlouv changed the title [APM] Display AWS and Azure icons [APM] Add AWS and Azure icons for additional services Jun 14, 2021
@sorenlouv sorenlouv force-pushed the add-dynamodb-icon branch from f4ac2f3 to 9788cb5 Compare June 14, 2021 12:30
@sorenlouv
Copy link
Member Author

@cauemarcondes Can you review again? Updated the PR with a storybook and additional icons.

@sorenlouv sorenlouv mentioned this pull request Jun 14, 2021
Closed
cauemarcondes
cauemarcondes reviewed Jun 14, 2021
View reviewed changes
x-pack/plugins/apm/public/components/shared/span_icon/get_span_icon.ts
aws: {
servicename: awsIcon,
servicename: 'logoAWS',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe since you're repeating logoAWS and logoAzure multiple times, would be a good idea to create a constant for them.

Suggested change
servicename: 'logoAWS',
servicename: LOGO_AWS,

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to invert it like:

Suggested change
servicename: 'logoAWS',
servicename: {
aws: LOGO_AWS
}

So we'd call <SpanIcon type="servicename" subtype="aws" />

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the better solution is that EUI exports this as a constant, so it can be used like:

servicename: ICONS.AWS

I've created an issue for this in the eui repo elastic/eui#4877

cauemarcondes
cauemarcondes approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM, just left two small comments that are up to you if you want to change it or not.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1600 1599 -1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 4.3MB 4.3MB -7.5KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@sorenlouv sorenlouv added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 14, 2021
@sorenlouv sorenlouv merged commit 970e9a0 into elastic:master Jun 14, 2021
@sorenlouv sorenlouv deleted the add-dynamodb-icon branch June 14, 2021 15:14
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 14, 2021
@sorenlouv @kibanamachine
[APM] Add AWS and Azure icons for additional services (elastic#101901)
4d4242e
@kibanamachine kibanamachine mentioned this pull request Jun 14, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

smith
smith reviewed Jun 14, 2021
View reviewed changes
x-pack/plugins/apm/public/components/shared/span_icon/span_icon.stories.tsx

const types = Object.keys(typeIcons);

storiesOf('shared/span_icon/span_icon', module)
Copy link
Contributor

Choose a reason for hiding this comment

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

@sqren we have a story for node icons: https://github.com/smith/kibana/blob/b307cee64e3bee0cb7bcda742c3f94f17d5d8efe/x-pack/plugins/apm/public/components/app/service_map/__stories__/Cytoscape.stories.tsx#L73-L296, so maybe we should delete that one.

Also we should prefer using Component Story Format over the legacy API.

Copy link
Member Author

Choose a reason for hiding this comment

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

From the Cytoscape story, I can't tell if we are show-casing the span icons (in which case it should just be deleted), or it's meant to show how it looks in the service map, in which case we should keep it.

It also looks like it's showing the agent icons via iconForNode so seems like it's covering more than the pure "span_icons" story.

Perhaps we can trim the cytoscape story down to not include every icon but just a few important ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I see now that's it's simply a grid, similar to the one I made

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, notice how "aws" is empty. It works under the story I added:
image

There seems to be a bug here. Interesting!

kibanamachine added a commit that referenced this pull request Jun 14, 2021
@kibanamachine @sorenlouv
[APM] Add AWS and Azure icons for additional services (#101901) (#102082
cb92d45 
)

Co-authored-by: Søren Louv-Jansen <sorenlouv@gmail.com>
@sorenlouv
Copy link
Member Author

sorenlouv commented Jun 15, 2021 •
edited
Loading

@smith I've created a follow-up PR #102182

cuff-links pushed a commit to cuff-links/kibana that referenced this pull request Jun 15, 2021
@sorenlouv
[APM] Add AWS and Azure icons for additional services (elastic#101901)
f0dc3ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smith smith smith left review comments

@cauemarcondes cauemarcondes cauemarcondes approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team:APM All issues that need APM UI Team support v7.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Add span icons for AWS and Azure services
5 participants
@sorenlouv @elasticmachine @kibanamachine @smith @cauemarcondes