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

Removing caller_pid from log entries break some integrations #2843

Closed
zachdaniel opened this issue Nov 26, 2018 · 4 comments
Closed

Removing caller_pid from log entries break some integrations #2843

zachdaniel opened this issue Nov 26, 2018 · 4 comments

Comments

@zachdaniel
Copy link
Contributor

In this commit: 8d09248#diff-152ab0727a1d450c0fe74d178e80d1f0

LogEntry had its caller_pid field removed. However, spandex_ecto (which has its own problems but we're going to resolve those), relies on that field being set to tie parent process trace data with the metrics in the log entry. It also seems like this information is not present in the telemetry information. Any suggestions on how we might go about getting query data into traces in a similar format? We don't want to have to wrap all calls to a repo in traces/spans.

@zachdaniel zachdaniel changed the title Removing caller_pid from log entries break integrations Removing caller_pid from log entries break some integrations Nov 26, 2018
@josevalim
Copy link
Member

The caller_pid is going to be self() in the majority of the cases, except on multi-preloads, which use separate queries. So you can just use self(). For the remaining cases, then elixir-lang/elixir#7995 will be the way to go, which should work generally and not only within Ecto.

@zachdaniel
Copy link
Contributor Author

Thanks, I look forward to seeing that make it in. That would be useful for our tracing library in many different ways.

@gmile
Copy link
Contributor

gmile commented Mar 1, 2019

@josevalim what would be a recommended way of having log metadata in the multi-preload queries now that Elixir 1.8 is out? Would you saw developers should handle this themselves or?

So far, this is what I arrived at to extra metadata from the caller process – would you say this is normal approach to have request_id during parallel-preloading:

require Logger

Logger.metadata(request_id: "03872725-6411-4746-acb8-047f10aa0072")

telemetry_fn = fn a,b,c,d ->
  case Process.info(self())[:dictionary][:"$callers"] do
    [caller_process] ->
      {:dictionary, stuff} = Process.info(caller_process, :dictionary)
      {true, log_metadata} = stuff[:logger_metadata]
      Logger.info("We're in a task process, preloading some assoc, request_id is #{log_metadata[:request_id]}")

    _ ->
      Logger.info("We've in a caller process itself")
  end
end

:telemetry.attach("my-handler", [:my_app, :repo, :query], telemetry_fn, nil)

# Test
post = Repo.insert!(%Post{author_id: 1})
Repo.insert!(%Comment{post_id: post.id, state: "approved"})
Repo.insert!(%Comment{post_id: post.id, state: "disapproved"})
Repo.insert!(%Comment{post_id: post.id, state: "pending"})
Repo.preload(post, [:comments, :author])

@josevalim
Copy link
Member

Exactly. Just replace [caller_process] -> by [caller_process | _] ->.

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

No branches or pull requests

3 participants