Skip to content

Instrument CLI with APM#75521

Closed
tylersmalley wants to merge 2 commits intoelastic:masterfrom
tylersmalley:devstats
Closed

Instrument CLI with APM#75521
tylersmalley wants to merge 2 commits intoelastic:masterfrom
tylersmalley:devstats

Conversation

@tylersmalley
Copy link
Copy Markdown
Member

@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
Copy link
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
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 <tyler.smalley@elastic.co>
@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 <tyler.smalley@elastic.co>
@kibanamachine
Copy link
Copy Markdown
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