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: Add telemetry middleware. #93

Merged
merged 2 commits into from
Feb 28, 2024
Merged

feat: Add telemetry middleware. #93

merged 2 commits into from
Feb 28, 2024

Conversation

jimsynz
Copy link
Contributor

@jimsynz jimsynz commented Feb 28, 2024

You can now add the Reactor.Middleware.Telemetry middleware to your reactors and it will publish telemetry events about the run.

@jimsynz jimsynz requested a review from zachdaniel February 28, 2024 03:40
@jimsynz jimsynz self-assigned this Feb 28, 2024

defp step_count(reactor) do
vertices = Graph.num_vertices(reactor.plan)
length(reactor.steps) + vertices
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why, but this seems strange to me. The number of steps is the vertices in the plan plus the steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. Any steps that have already been planned will be in the graph, whereas any unplanned steps are stored in the steps list until the next loop through the reactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we store a step count in the state as steps are added so that we don't have to do lots of counting? I'm not sure if num_vertices/1 is constant time or not, but if not we might want to consider keeping both counts in the state. Probably not big lists, but also if we count every time for every invocation of every middleware, it could end up being lots of calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty sure it just calls map_size behind the scenes. we could cache it, but since step_count is only called by the init callback, it's not happening very often. 🤷

@jimsynz jimsynz merged commit 37b9eda into main Feb 28, 2024
13 checks passed
@jimsynz jimsynz deleted the feat/telemetry-middleware branch February 28, 2024 20:23
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.

2 participants