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.

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

[KP] instrument platform server-side code with apm agent #70919

Merged
merged 8 commits into from
Oct 1, 2020

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Jul 7, 2020

Summary

Adds tracking for setup & start lifecycles of Kibana server + SO migration operation.

How to test locally:

For maintainers

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 7, 2020
@mshustov mshustov requested a review from a team as a code owner July 7, 2020 07:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@mshustov
Copy link
Contributor Author

mshustov commented Jul 7, 2020

@vigneshshanmugam Will we be able to see the added APM metrics in the Cloud? Or additional setup required?

@vigneshshanmugam
Copy link
Member

@restrry Are you referring to Metrics captured by the Node.js agent? You would be able to see them under Metrics tab in API UI.

Screenshot 2020-07-07 at 11 31 19

@mshustov
Copy link
Contributor Author

mshustov commented Jul 7, 2020

@vigneshshanmugam yeah, I tested it locally. I'm wondering when I can see them in APM for Cloud Dev env?

@vigneshshanmugam
Copy link
Member

@restrry Ah, got it. Seems like the clod is on 7.7 version, I will ask the team to update to the latest stack and you should be able to see it.

Comment on lines 437 to 439
if (migrationSpan) {
migrationSpan.end();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: migrationSpan?.end(); (multiple occurences)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for this, we need to turn off eslint/no-unused-expressions and use @typescript-eslint/no-unused-expressions instead.
typescript-eslint/typescript-eslint#1220

@pgayvallet
Copy link
Contributor

Tested locally. Found the added transactions and spans, seems to be working as intended.

Screenshot 2020-07-08 at 10 20 11

Screenshot 2020-07-08 at 10 22 11

Remark:

apm.startSpan('saved_objects', 'migration'); seems to be creating a span with name saved_objects and type migration:

Screenshot 2020-07-08 at 10 25 16

Just want to be sure that it really is what we want.

Question:

Are plugins supposed to be able to open a transaction during their setup or start phase? If so, are nested transactions a thing, as we already have a higher level transaction for core's setup and start.

@mshustov
Copy link
Contributor Author

mshustov commented Jul 8, 2020

Are plugins supposed to be able to open a transaction during their setup or start phase? If so, are nested transactions a thing, as we already have a higher level transaction for core's setup and start.

I'd expect so, because Transactions are a special kind of span that have additional attributes associated with them.

seems to be creating a span with name saved_objects and type migration:

I didn't test this after the change. I will roll it back to apm.startSpan('saved_objects.migration', 'migration');

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM

@mshustov
Copy link
Contributor Author

@elasticmachine merge upstream

@mshustov mshustov added v7.10.0 and removed v7.9.0 labels Jul 16, 2020
@joshdover
Copy link
Contributor

@restrry could we get this in for 7.10? I'd love to include these metrics in our first iteration of APM on Cloud

@mshustov
Copy link
Contributor Author

@joshdover sure, I will update the branch

@mshustov mshustov force-pushed the instrument-platform-code-with-apm branch from 7ed70cf to 0b091a5 Compare September 29, 2020 13:50
@mshustov
Copy link
Contributor Author

updated

@mshustov
Copy link
Contributor Author

@elasticmachine merge upstream

@mshustov
Copy link
Contributor Author

@elasticmachine merge upstream

2 similar comments
@mistic
Copy link
Member

mistic commented Sep 30, 2020

@elasticmachine merge upstream

@mshustov
Copy link
Contributor Author

@elasticmachine merge upstream

@mshustov mshustov force-pushed the instrument-platform-code-with-apm branch from 14e54aa to 892a5a9 Compare September 30, 2020 18:02
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@mshustov mshustov merged commit 3024513 into elastic:master Oct 1, 2020
@mshustov mshustov deleted the instrument-platform-code-with-apm branch October 1, 2020 11:06
mshustov added a commit to mshustov/kibana that referenced this pull request Oct 1, 2020
* instrument platform server-side code with apm agent: setup, start lifecycles and SO migration

* add span type

* span should have name: saved_objects.migration

* remove migration reports

* put migration span back
mshustov added a commit that referenced this pull request Oct 1, 2020
…9097)

* instrument platform server-side code with apm agent: setup, start lifecycles and SO migration

* add span type

* span should have name: saved_objects.migration

* remove migration reports

* put migration span back
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants