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.

Sign up for GitHub

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

fix: Ensure shape info is added to OTEL spans in shape requests #2358

Merged
merged 4 commits into from
Feb 20, 2025
+36 −11
Merged

fix: Ensure shape info is added to OTEL spans in shape requests #2358

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb86536
Fix OTEL shape properties
msfstef Feb 19, 2025
cfeb601
Add changeset
msfstef Feb 19, 2025
d93d64e
Properly serialize columns
msfstef Feb 19, 2025
63ea03f
Add `xid` to `pg_txn.replication_client.transaction_received`
msfstef Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wet-zebras-laugh.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@core/sync-service": patch
---

Ensure shape properties are added to OTEL spans in shape requests.
36 changes: 26 additions & 10 deletions packages/sync-service/lib/electric/plug/serve_shape_plug.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,24 +153,40 @@ defmodule Electric.Plug.ServeShapePlug do
params = Map.get(request, :params, %{}) |> bare_map()
response = (Map.get(assigns, :response) || Map.get(request, :response) || %{}) |> bare_map()
attrs = Map.get(response, :trace_attrs, %{})
maybe_up_to_date = Map.get(response, :up_to_date, false)

shape_handle =
conn.query_params["handle"] || params[:handle] || request[:handle]
is_live_req =
if is_nil(params[:live]),
do: !is_nil(conn.query_params["live"]) && conn.query_params["live"] != "false",
else: params[:live]

maybe_up_to_date = Map.get(response, :up_to_date, false)
replica =
Copy link
Contributor

Choose a reason for hiding this comment

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

we have the actual served shape in the request in the assigns, specifically conn.assigns.request.params.{shape_definition, columns, replica}

this will have the final resolved info like replica, table, where etc

and the conn.assigns.request has the shape handle conn.assigns.request.handle and other stuff

using these might be simpler since you get the resolved value after applying defaults etc.

Copy link
Contributor Author

@msfstef msfstef Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That is what I'm using - this is what params is, and all values use that as well. There are two attempts to populate these, one at the start of the request and one at the end.

My question is that that because request might fail before these are resolved, we would still want to have these attributes populated, so should we prioritise the resolved ones once they are available or the requested ones?

Also there's a difference between a request explicitly requesting replica=default and a request that will use defaults

Copy link
Contributor

Choose a reason for hiding this comment

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

That is what I'm using - this is what params is, and all values use that as well

my bad! sorry, should have read higher up, especially as I think those earlier lines are mine!

My question is that that because request might fail before these are resolved, we would still want to have these attributes populated, so should we prioritise the resolved ones once they are available or the requested ones?

I guess if the request failed, we'd want the original values so we can maybe see what the problem is, but if it succeeded then we'd want the resolved ones.

or alternatively we keep the original params available and also include the resolved version if available.

that might be simpler -- always put in the original params but only include the resolved ones if they exist.

that would help with the issues around replica you mention.

if is_nil(params[:replica]),
do: conn.query_params["replica"],
else: to_string(params[:replica])

columns =
if is_nil(params[:columns]),
do: conn.query_params["columns"],
else: Enum.join(params[:columns], ",")

Electric.Telemetry.OpenTelemetry.get_stack_span_attrs(
get_in(conn.assigns, [:config, :stack_id])
)
|> Map.merge(Electric.Plug.Utils.common_open_telemetry_attrs(conn))
|> Map.merge(%{
"shape.handle" => shape_handle,
"shape.where" => get_in(params, [:shape, :where]),
"shape.root_table" => get_in(params, [:shape, :root_table]),
"shape.definition" => get_in(params, [:shape]),
"shape.replica" => get_in(params, [:shape, :replica]),
"shape_req.is_live" => params[:live],
"shape_req.offset" => params[:offset],
"shape.handle" => conn.query_params["handle"] || params[:handle] || request[:handle],
"shape.where" => conn.query_params["where"] || params[:where],
"shape.root_table" => conn.query_params["table"] || params[:table],
"shape.columns" => columns,
# # Very verbose info to add to spans - keep out unless we explicitly need it
# "shape.definition" =>
# if(not is_nil(params[:shape_definition]),
# do: Electric.Shapes.Shape.to_json_safe(params[:shape_definition])
# ),
"shape.replica" => replica,
"shape_req.is_live" => is_live_req,
"shape_req.offset" => conn.query_params["offset"],
"shape_req.is_shape_rotated" => attrs[:ot_is_shape_rotated] || false,
"shape_req.is_long_poll_timeout" => attrs[:ot_is_long_poll_timeout] || false,
"shape_req.is_empty_response" => attrs[:ot_is_empty_response] || false,
Expand Down
Loading