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

feat(apm): Heartbeat #2478

Merged
merged 7 commits into from
Mar 9, 2020
Merged

feat(apm): Heartbeat #2478

merged 7 commits into from
Mar 9, 2020

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Mar 9, 2020

Add a simple heartbeat check, if activities don't change in 3 beats, finish the transaction.

@HazAT HazAT requested a review from kamilogorek as a code owner March 9, 2020 11:25
@HazAT HazAT changed the title feat[apm]: Heartbeat feat(apm): Heartbeat Mar 9, 2020
packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved
clearTimeout(Tracing._heartbeat);
const keys = Object.keys(Tracing._activities);
if (keys.length) {
const heartbeatString = keys.reduce((prev: string, current: string) => prev + current);
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't it be too fragile?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure was looking for something reasonable of "hashing" this whole thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, it's based on keys, so id, not type of activity. It shooooould be good enough for now then.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be unnecessarily large (it is a string of unbounded length). Plus it sort of relies on key order. Looking for something better...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, isn't this generating a lot of garbage for the GC every 5s?

Copy link
Contributor

Choose a reason for hiding this comment

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

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved
Co-Authored-By: Kamil Ogórek <kamil.ogorek@gmail.com>
Tracing._addPerformanceEntries(active);
logger.log('[Tracing] finishIdleTransaction', active.transaction);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this moving down?

Copy link
Member Author

Choose a reason for hiding this comment

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

So debug prints are not confusing, that finish is the last thing we print before flushing the transaction.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved
logger.log(
"[Tracing] Heartbeat safeguard kicked in, finishing transaction since activities content hasn't changed for 3 beats",
);
Tracing._activeTransaction.setStatus(SpanStatus.DeadlineExceeded);
Copy link
Contributor

Choose a reason for hiding this comment

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

This status is the same as set by fromHttpCode for responses with 504 code.
Does it make sense to reuse the same status here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't it be confusing the UI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, you are not wrong but I thought we need to mark this transaction somehow and we also do it already for spans where autopop kicks in

HazAT and others added 2 commits March 9, 2020 16:44
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Just FTR I'm okay with this going in, it is a neat idea. As we other things tracing, we iterate and refine it.
Looking forward to see what heartbeat can do to traces.

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