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

traceparent or X-Opaque-Id headers to query tags #2506

Open
steve-chavez opened this issue Oct 11, 2022 · 9 comments
Open

traceparent or X-Opaque-Id headers to query tags #2506

steve-chavez opened this issue Oct 11, 2022 · 9 comments
Labels
enhancement a feature, ready for implementation logging observability

Comments

@steve-chavez
Copy link
Member

steve-chavez commented Oct 11, 2022

Query tags are comments on the SQL query(SELECT ... FROM tbl /*key=value/*) , they're useful for getting context on the database logs.

References:

We could include

  • A request_id(request_id=...). It could be generated by us or configured to be obtained from a header since a proxy already adds a request id: nginx has request_id, cloudflare has cf-ray, etc. We could also include this in the response header(configurable), to make it easier for users to search the generated query in the logs.
  • The HTTP method(request_method=GET)
  • The path (request_path=tbl)

It should be configurable

db-query-tags = true
log-request-id = generated|arbitrary-header-name # could be `cf-ray`, `x-request-id`, etc
@steve-chavez steve-chavez added idea Needs of discussion to become an enhancement, not ready for implementation observability labels Oct 11, 2022
@steve-chavez
Copy link
Member Author

Just found out that there is a standard HTTP header for doing the above: traceparent.

References:

I found it on the Elasticsearch docs, which uses an X-Opaque-Id for the same purpose.

Also, the tracestate header allows key value pairs for extra vendor information.

We could include this information on the query tags.


One problem with traceparent is that it has its own format

traceparent: 00-80e1afed08e019fc1110464cfa66635c-7a085853722dc6d2-01

The traceparent header uses the version-trace_id-parent_id-trace_flags format where:

version is always 00.
trace_id is a hex-encoded trace id.
span_id is a hex-encoded span id.
trace_flags is a hex-encoded 8-bit field that contains tracing flags such as sampling, trace level, etc.

Though trace_id is 16 bytes and matches the length of nginx's request_id and cloudflare cf-ray, those systems would still have to send a modified header for us to consume.

It also requires parsing and validating the header value as mentioned on the docs.

On contrast the X-Opaque-Id convention can have an arbitrary value inside. It might be simpler to adopt this one for starters.

@steve-chavez steve-chavez changed the title add query tags traceparent or X-Opaque-Id to query tags Nov 9, 2022
@steve-chavez steve-chavez changed the title traceparent or X-Opaque-Id to query tags traceparent or X-Opaque-Id headers to query tags Nov 9, 2022
@steve-chavez
Copy link
Member Author

There's also the x-request-id header.

With these variety of options think it'd be easier to just make it configurable.

So server-trace-id-header as config, empty by default.

@steve-chavez steve-chavez added enhancement a feature, ready for implementation and removed idea Needs of discussion to become an enhancement, not ready for implementation labels Feb 27, 2023
@steve-chavez
Copy link
Member Author

A server-trace-header config was added on #2692.

@wolfgangwalther
Copy link
Member

Should we somehow add the value passed through this header to the request logs, too?

@steve-chavez
Copy link
Member Author

@wolfgangwalther What would the format look like?

Also, how would that help with observability? Right now I'm mostly thinking of parsing the db logs.

@wolfgangwalther
Copy link
Member

What would the format look like?

No idea, yet. It probably doesn't fit CLF, we'd might need Extended Log Format.

Also, how would that help with observability? Right now I'm mostly thinking of parsing the db logs.

I had some cases where I wasn't sure which component (nginx, postgrest, postgresql) was actually throwing an error, when I got some failed requests on the client. It would be nice to just put that trace header on the request on the client and then grep all logs to see where it's logged and where it's not. I also had some cases where I wanted to track 2xx responses, while setting up nginx to cache responses. In those cases it would have been easier to see whether nginx was responding directly from cache or was hitting PostgREST.

@elimisteve
Copy link
Contributor

So server-trace-id-header as config, empty by default.

To be clear, what was implemented as new a new config option is called server-trace-header, not server-trace-id-header.

@olee
Copy link

olee commented Mar 21, 2024

Should we somehow add the value passed through this header to the request logs, too?

That would actually be really nice. Right now there doesn't seem to be any way to create full trace logs from frontend to backend because there's no way to identify the request log in postgrest with the requests from frontend / elsewhere.

@steve-chavez
Copy link
Member Author

Related to #3415 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a feature, ready for implementation logging observability
Development

No branches or pull requests

4 participants