Skip to content

[APM] Set a valid service_name for python APM onboarding#131959

Merged
basepi merged 5 commits intoelastic:mainfrom
basepi:django_flask_apm_onboarding
May 11, 2022
Merged

[APM] Set a valid service_name for python APM onboarding#131959
basepi merged 5 commits intoelastic:mainfrom
basepi:django_flask_apm_onboarding

Conversation

@basepi
Copy link
Copy Markdown
Contributor

@basepi basepi commented May 10, 2022

Summary

A user noticed that we were recommending an empty string for SERVICE_NAME in our python APM onboarding. This is invalid and will result in no data showing up in the APM app.

I have fixed the sample config to include a generic service name.

Checklist

No items were applicable to this change.

For maintainers

@basepi basepi requested a review from a team May 10, 2022 18:01
@botelastic botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label May 10, 2022
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 10, 2022

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

cauemarcondes
cauemarcondes approved these changes May 10, 2022
View reviewed changes
Copy link
Copy Markdown
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. But you'll need to fix some unit tests like: https://github.com/elastic/kibana/blob/main/x-pack/plugins/apm/public/tutorial/config_agent/commands/get_commands.test.ts#L184

@basepi basepi enabled auto-merge (squash) May 10, 2022 18:24
@cauemarcondes
Copy link
Copy Markdown
Contributor

cauemarcondes commented May 10, 2022

@elasticmachine merge upstream

@basepi basepi disabled auto-merge May 11, 2022 16:13
@basepi basepi requested review from stuartnelson3 and trentm May 11, 2022 16:14
@basepi
Copy link
Copy Markdown
Contributor Author

basepi commented May 11, 2022 •
edited
Loading

@trentm @stuartnelson3 I took the liberty of adding a SERVICE_NAME to the node/go configuration samples. If that change doesn't make sense for either of your agents, please tell me!

trentm
trentm reviewed May 11, 2022
View reviewed changes
@basepi basepi mentioned this pull request May 11, 2022
Merged
@basepi basepi removed the request for review from stuartnelson3 May 11, 2022 16:52
@basepi
Copy link
Copy Markdown
Contributor Author

basepi commented May 11, 2022

@trentm @stuartnelson3 I'm reverting those changes, keep things simple. I'm guessing go deals with the empty string similar to Node. If not, you know where you can change it! :)

@kibana-ci
Copy link
Copy Markdown

kibana-ci commented May 11, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
apm 2.8MB 2.8MB +34.0B

History

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

@basepi basepi merged commit aa07c76 into elastic:main May 11, 2022
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label May 11, 2022
@basepi basepi mentioned this pull request May 17, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trentm trentm trentm left review comments

@cauemarcondes cauemarcondes cauemarcondes approved these changes

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting release_note:fix Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v8.3.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@basepi @elasticmachine @cauemarcondes @kibana-ci @trentm @kibanamachine