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

Instrument CLI with APM #75521

Closed
wants to merge 2 commits into from
Closed

Conversation

tylersmalley
Copy link
Contributor

@tylersmalley tylersmalley commented Aug 20, 2020

As we work to improve the developer experience, it's important that we track the time of bootstrap and optimizer. With this, we can also ensure that developers across operating systems and machines are receiving a similar expereince.

  • Move apm shared config to a package so it can be shared

@tylersmalley tylersmalley force-pushed the devstats branch 5 times, most recently from c7daa85 to 37671dc Compare August 20, 2020 15:13
await apm.flush();

// TODO: It appears the cb is called BEFORE the API request in flush
await new Promise((resolve) => setTimeout(resolve, 1000));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vigneshshanmugam, this is the issue I mentioned previously. I haven't dug into it too deeply, but it appears the callback to flush is called before the HTTP request is made. Is this something you could look into? A 1-second wait is obviously something we don't want to merge with.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I will have a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Were you able to reproduce?

Copy link
Member

Choose a reason for hiding this comment

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

Was busy with other stuffs, Apologies for the delay. Will get back to it soon. Is it blocking the current PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, yeah we will need to resolve that before moving forward with this. We're currently working to have APM collection for local development, CI and on Cloud.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, Had a quick look and it seems like since there is no active request to the APM server, the callback is getting immediately invoked as a result. I will debug more on how we can tune it to report events from every CLI run.

Copy link
Member

Choose a reason for hiding this comment

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

Btw, I did try a few runs and all of the events were reported properly to the server even without the setTimeout hack as the underlying transport layer guarantees the transaction was written to the stream when trans.end() is called and flush guarantees the stream is closed properly.

Do we need to do anything else here?

Signed-off-by: Tyler Smalley <[email protected]>
@tylersmalley tylersmalley force-pushed the devstats branch 5 times, most recently from 7fd07f3 to 88ad854 Compare September 1, 2020 20:31
Signed-off-by: Tyler Smalley <[email protected]>
@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 1, 2020

💔 Build Failed

Failed CI Steps

Build metrics

‼️ metrics were not reported for [#75521@45b86cd]

History

  • 💔 Build #71708 failed 88ad8544a7a9720b7599aedc429c6a53145c3231
  • 💔 Build #71704 failed 7fd07f3b2ce3ec6798a6cc2502c2e8d932773f09
  • 💔 Build #71696 failed 5bdbfcd66fa6b8089a5f83c7259e42a703b59718
  • 💔 Build #71653 failed b38a068882f57d75e99f0cf3f5e0e41f7a60a633

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants