Skip to content

use private functions as report_cb#775

Open
martosaur wants to merge 1 commit into
elixir-ecto:masterfrom
martosaur:am-non-local-report-cb
Open

use private functions as report_cb#775
martosaur wants to merge 1 commit into
elixir-ecto:masterfrom
martosaur:am-non-local-report-cb

Conversation

@martosaur

Copy link
Copy Markdown
Contributor

@hauleth raised a good point that anonymous functions shouldn't be used as report callbacks. This PR uses private functions instead. Embarrassingly, this isn't the first time I'm making this mistake.

Anyways, I went with a private function per log message, but open to other arrangements (separate module? a single function with multiple heads matching on log_id?) if you think it's better.

@josevalim

Copy link
Copy Markdown
Member

@hauleth does it need to be a public function too? IIRC yes but double checking!

@hauleth

hauleth commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Yeah, it needs to be public function and capture needs to use remote call format.

Personally I use such approach most of the time:

# Do not show in documentation
@doc false
# underscore functions aren't imported by default AFAIK
def __report_cb__(report) do
  # ...
end

That way it is public function from VM viewpoint, but it is private API from documentation viewpoint.

@martosaur

Copy link
Copy Markdown
Contributor Author

I'll make the change, but at this point I have to ask for more details. What is that quirk we're talking about and where can I read about it? It sounds like it'd be nice to document it somewhere if reports are to get any adoption.

@martosaur

Copy link
Copy Markdown
Contributor Author

Ok I think I found where it's coming from. Telemetry recommends remote captures for performance reasons, but looking at (this thread)[https://github.com/elixir-lang/elixir/pull/13668] it looks like the eventual conclusion was that performance difference might not be noticeable with recent OTP versions. So the only justification that landed in the docs is hot code reload. Which is still a good enough reason to use remote captures!

@martosaur martosaur force-pushed the am-non-local-report-cb branch from bbb2250 to feb571a Compare July 1, 2026 18:12
@martosaur

Copy link
Copy Markdown
Contributor Author

Pushed the change! I didn't spot any difference from underscores (as tested with ModuleName.module_info(:exports)) so didn't use them.

@hauleth

hauleth commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@martosaur martosaur force-pushed the am-non-local-report-cb branch from feb571a to a43913a Compare July 1, 2026 19:31
@martosaur

Copy link
Copy Markdown
Contributor Author

oof i misread "import" for "export" 🤦 ok, thanks for bearing with me folks, this new push should finally be good!

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