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] Added Android Agent name and icon #136598

Merged
merged 10 commits into from
Jul 19, 2022
Merged

[APM] Added Android Agent name and icon #136598

merged 10 commits into from
Jul 19, 2022

Conversation

AlexanderWert
Copy link
Member

@AlexanderWert AlexanderWert commented Jul 19, 2022 •
edited
Loading

Summary

  • Added new agent name for the APM Android agent
  • Added Android icon for the APM Android agent.
  • Added agent name to telemetry task
  • Replaced usage of isIOSAgentName with isMobileAgentName in the APM UI

For maintainers

@AlexanderWert AlexanderWert added the Team:APM All issues that need APM UI Team support label Jul 19, 2022
@AlexanderWert AlexanderWert requested a review from a team as a code owner July 19, 2022 07:20
@elasticmachine
Copy link
Contributor

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

@AlexanderWert AlexanderWert changed the title Added Android Agent icon [APM] Added Android Agent icon Jul 19, 2022
@AlexanderWert AlexanderWert requested a review from boriskirov July 19, 2022 07:21
@AlexanderWert AlexanderWert changed the title [APM] Added Android Agent icon [APM] Added Android Agent name and icon Jul 19, 2022
gbamparop
gbamparop reviewed Jul 19, 2022
View reviewed changes
x-pack/plugins/apm/common/agent_name.ts Outdated
export function isRumAgentName(
agentName?: string
): agentName is 'js-base' | 'rum-js' | 'opentelemetry/webjs' {
return RUM_AGENT_NAMES.includes(agentName! as AgentName);
}

export function isMobileAgentName(
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be used

Copy link
Member Author

@AlexanderWert AlexanderWert Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @gbamparop !

I pushed a commit now in which I replaced the usage of isOSAgentName with isMobileAgentName in all the places in the APM UI. Since the Android agent should be treated in the same way as the iOS agent.

Copy link
Member Author

@AlexanderWert AlexanderWert Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What is not clear to me is:

  • is the method normalizeAgentName used anywhere? I couldn't find anything
  • why did we define opentelemetry/swift to be an iOS agent / mobile agent. We might need to change this as well. Because if a MacOS client application is instrumented with the OpenTelemetry SDK, this would be recognized and treated as a mobile agent. I don't think this is what we want. Do have any concerns with changing this as well (so that only iOS/swift or ios/swift would be recognized as iOSAgent / mobile agent)?
  • Why do we hide the dependencies view for the iOSAgent / mobile agent?

Copy link
Contributor

@gbamparop gbamparop Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for updating this!

  • is the method normalizeAgentName used anywhere? I couldn't find anything

It seems to have been introduced by #102346 but it's not used

  • why did we define opentelemetry/swift

It makes sense, maybe we hadn't considered that at the time (see #103485 (comment))

It was raised as an issue #108510

Copy link
Member Author

@AlexanderWert AlexanderWert Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @gbamparop

  • is the method normalizeAgentName used anywhere? I couldn't find anything

It seems to have been introduced by #102346 but it's not used

OK, I deleted it and simplified the code there.

  • why did we define opentelemetry/swift

It makes sense, maybe we hadn't considered that at the time

👍 I reverted it to be just iOS/swift / ios/swift

It was raised as an issue #108510

I'll keep it as is for now and continue the discussion on this aspect in a separate thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, do you think there's another field that we could use to find if an app instrumented with opentelemetry/swift is on iOS?

@AlexanderWert AlexanderWert requested review from a team as code owners July 19, 2022 08:58
gbamparop
gbamparop approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@gbamparop gbamparop left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for picking this up!

@AlexanderWert AlexanderWert added release_note:skip Skip the PR/issue when compiling release notes v8.4.0 labels Jul 19, 2022
@AlexanderWert AlexanderWert enabled auto-merge (squash) July 19, 2022 14:46
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1304 1305 +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 3.0MB 3.0MB +967.0B

History

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

@AlexanderWert AlexanderWert merged commit 6af683a into elastic:main Jul 19, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 19, 2022
@LikeTheSalad LikeTheSalad mentioned this pull request Nov 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbamparop gbamparop gbamparop approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

@boriskirov boriskirov Awaiting requested review from boriskirov

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AlexanderWert @elasticmachine @kibana-ci @pgayvallet @gbamparop @kibanamachine