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

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

Merged
merged 4 commits into from
Feb 20, 2025

Conversation

msfstef
Copy link
Contributor

@msfstef msfstef commented Feb 19, 2025

Fixes https://github.com/electric-sql/stratovolt/issues/299

It's a bit confusing because we use the same attirbute names elsewhere and use different values (e.g. "shape.root_table" is also added as a span in Electric.Shapes.Consumer, but there it uses the namespaced tuple instead of the request provided table).

Right now they are coming in as nil because there is no such thing as a :shape key in the parameters, so I'm changing this to at least always try and include something.

This populating of attributes happens both at the start of the request and at the end - I always try to use the raw query parameter value, because it should always be available and it is what we will try to correlate to request spans from proxies etc. I have the parsed parameter fallback but I'm not sure if that is even necessary or if it makes sense to be there.

For example, for the replica parameter, if it is not present in the query string, it will still be populated with the parsed value which defaults to "default" - but for the purpose of this span, should we not leave it as nil if the requester didn't include it? Or should we include what we parsed the request as? And should priority go to the parsed version or the raw version?

I've also removed the shape.definition attribute as it feels a bit too verbose - we can technically add it with a json safe version of the shape definition, but it feels like overkill.

Unsure about these changes but figured it'd be good to at least get the basics working again - this would benefit from tests but I think we need an OTEL dependency injection to get it working, if anyone has a better idea I'd appreciate that.

Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@magnetised magnetised left a comment

Choose a reason for hiding this comment

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

I think it's worth having a look at using the values in the resolved Shapes.Api.Request struct -- see my comment


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

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.

@msfstef msfstef merged commit 8a4b0d5 into main Feb 20, 2025
34 checks passed
@msfstef msfstef deleted the msfstef/fix-ot-span-shape-attrs branch February 20, 2025 11:49
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