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

Telemetry/timestamps and spans #162

Merged
merged 9 commits into from
Dec 14, 2023
Merged

Conversation

NelsonVides
Copy link
Collaborator

I'm going to want timestamps for all these telemetry events, so that metrics collectors can aggregate and draw plots based on time.

I also want an event for scenario start and stop which will mimic telemetry's span mechanism, but which here it is done in this way because the controller separately calls the beginning and the end of the scenario and span wouldn't fit into this mechanism.

Split from #155

@NelsonVides NelsonVides mentioned this pull request Dec 2, 2023
@NelsonVides NelsonVides force-pushed the telemetry/timestamps_and_spans branch 2 times, most recently from d4272a0 to 2b47091 Compare December 14, 2023 12:10
guides/telemetry.md Outdated Show resolved Hide resolved
guides/telemetry.md Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (master@fd29e7e). Click here to learn what that means.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #162   +/-   ##
=========================================
  Coverage          ?   33.23%           
=========================================
  Files             ?       23           
  Lines             ?      975           
  Branches          ?        0           
=========================================
  Hits              ?      324           
  Misses            ?      651           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -2,63 +2,67 @@ Amoc also exposes the following telemetry events:

## Scenario

A telemetry span of a full scenario execution
A telemetry span of a full scenario execution globally (i.e. the exported `init/0` function):
Copy link
Collaborator

Choose a reason for hiding this comment

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

i.e. the exported init/0 function - this is incorrect, it's rather a span between init/1 and terminate/1 call

also I don't think it's a good idea to manually emulate a span here. because scenario doesn't end itself, it finishes per user request. so duration doesn't indicate real scenario execution here. I would rather rework it into 2 events [amoc, scenario, start|stop] with duration reported for stop event. also reporting error during Scenario:termination/1 call as scenario exeption is incorrect. I think it makes sense to report every scenario callback as telemetry span (and we should do it from amoc_scenario module), and amoc_contorller should only issue telemetry events.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree to everything except the idea of running Scenario:start/ as a telemetry span in the amoc_scenario module, as this will run in the controller process and measure the time it takes to spawn an erlang process only, instead of measuring the lifespan of the entire amoc user. The current span must stay where it is already and it is the most useful one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what you mean here, I propose to measure exactly scenario callback time:
https://github.com/esl/amoc/blob/master/src/amoc_scenario.erl#L64

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, all of the callbacks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oooooh, I see, my bad! Ok, one moment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

and amoc_scenario module works as a synchronous wrapper for the callbacks. it doesn't spawn anything.

@NelsonVides NelsonVides force-pushed the telemetry/timestamps_and_spans branch 2 times, most recently from 6e47536 to b834956 Compare December 14, 2023 13:45
Copy link
Collaborator

@DenysGonchar DenysGonchar left a comment

Choose a reason for hiding this comment

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

great PR, thanks for addressing all the comments.

@DenysGonchar DenysGonchar merged commit 2aa65ba into master Dec 14, 2023
6 checks passed
@DenysGonchar DenysGonchar deleted the telemetry/timestamps_and_spans branch December 14, 2023 14:03
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