Skip to content

Added supervisor init telemetry event#223

Merged
josevalim merged 4 commits into
elixir-broadway:masterfrom
akoutmos:adding_supervisor_init_telemetry_event
Mar 16, 2021
Merged

Added supervisor init telemetry event#223
josevalim merged 4 commits into
elixir-broadway:masterfrom
akoutmos:adding_supervisor_init_telemetry_event

Conversation

@akoutmos
Copy link
Copy Markdown
Contributor

I am currently working on the PromEx Broadway plugin (akoutmos/prom_ex#39) and would like to incorporate some of the configuration details into the Broadway Grafana dashboard. Thoughts on surfacing the config from the Topology module after the supervisor is started?

If you are okay with this approach I will go through and add the necessary documentation to the telemetry events. Thanks in advance!

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 15, 2021

Pull Request Test Coverage Report for Build 778087c8823a988031750c90b547000269a981a7-PR-223

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 15 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 94.885%

Files with Coverage Reduction New Missed Lines %
lib/broadway/topology/producer_stage.ex 15 86.32%
Totals Coverage Status
Change from base Build 05f1dfc3461730a57e7942c4b623f3d70e56163e: 0.06%
Covered Lines: 538
Relevant Lines: 567

💛 - Coveralls

@josevalim
Copy link
Copy Markdown
Contributor

Hi @akoutmos! This looks good to me but I am afraid the configuration is somewhat private. We may tweak it so it mirrors our internals and that may break the dashboard. We should either expose specific information that you need or expose the user supplied map as is.

@akoutmos
Copy link
Copy Markdown
Contributor Author

I'm ok with using the user supplied map as opposed to the internal representation. It looks like the user's options are going through NimbleOptions so there shouldn't be a case where they think they set something, PromEx reports that something was set, but it turns out that their setting was ignored due to invalid option structure.

I'll update the PR to leverage the user options and also update the telemetry docs to include this new event. Thanks for the speedy feedback!

@akoutmos
Copy link
Copy Markdown
Contributor Author

Updated the PR to address your feedback. Let me know if there is anything else that i should adjust :).

Copy link
Copy Markdown

@philss philss left a comment

Choose a reason for hiding this comment

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

Great addition!
I left one suggestion :)

Comment thread lib/broadway.ex Outdated

Broadway currently exposes following Telemetry events:

* `[:broadway, :supervisor, :init]` - Dispatched when the supervision tree
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just a nitpick: instead of supervisor, we could name this topology or pipeline for better proximity with naming from Broadway. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great idea! I liked topology better and updated the PR with that one :D

@josevalim josevalim merged commit 258ac73 into elixir-broadway:master Mar 16, 2021
@josevalim
Copy link
Copy Markdown
Contributor

💚 💙 💜 💛 ❤️

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.

4 participants