-
Notifications
You must be signed in to change notification settings - Fork 12
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
Expose HTTP headers in CloudEvent Signature #34
Comments
Do you mean just adding them as attributes to the CloudEvent? I'm not sure that that's a good idea, if they're not logically part of the CloudEvent. Suppose the function wants to pass the CloudEvent on to something else to do the processing - if we've added extra things in that really aren't part of the CloudEvent, the function would need to strip those out first, which seems unfortunate. I think I'd prefer to have a separate parameter in the event signature, although that ends up being quite long-winded I suspect. Can we gather specific use cases for this? We could then try to come up with "what would the function author want to do in each case" examples. Note that tracing is particularly interesting as there are two potential trace contexts - the trace context of the event itself (e.g. "a GCS object was finalized while processing record X, which was created due to user activity Y"), and the trace context of event propagation. We should work out what needs to happen with each of those, and how they're propagated. |
Yeah, I'd probably not want them in the CE parameter as it can look and work not the best (need to try it out). Things like trace might be relevant in a CE object. There's a documented extension: See the internal GCF APM PRD. |
Thinking about this, I have to agree that it doesn't make sense to expose all/arbitrary headers, as they are part of the transport, not part of the CloudEvent itself. (We should not be adding fields to a CE that were not set by the sender.) I also don't know why a function should care about headers in the general case. Functions deal with logical events, and the fact that it came via HTTP vs something else should be abstracted away. That said, for the legacy event conversion, it probably does make sense to augment the conversion algorithm to include some additional information, especially where there's a documented extension such as distributed tracing. If an extension exists, I would assume a use case exists that drove the extension. But if there isn't a well-known extension, I'd still be hesitant. Adding any arbitrary header may be problematic—even if none of the headers clash with standard fields now, doesn't mean they won't in the future. |
Beyond collision, it expands the contract of CloudEvents to include more of the transport layer, where it's not unreasonable to imagine the transport layer might evolve at a different pace.
+1 to this. In some cases those headers are what CloudEvents are meant to replace through it's own standard (e.g., function-execution-id). In others the headers are a means to an end. Which of those ends apply and should be built into the FF+CloudEvents contract? This is a good meta issue, but potential header values to include should be considered one at a time. |
While doing a v0.3-v1.0 comparison of the CloudEvents spec (fun times, y'all!) I found this interesting paragraph:
That would suggest that the various CloudEvents SDKs might want to consider supporting this in the first instance, at which point it should be easy to integrate into the Functions Frameworks. |
@jskeet I did not notice that paragraph. That's definitely suggestive, although I'm not clear what exactly it means. I'll ask in the CloudEvents SDK slack if anyone is doing this and what it looks like. |
So I asked in the CE SDK slack, and people agree that the paragraph in the spec sounds somewhat misleading. The intent was to address extension attributes. It means, a CE process is not required to recognize and propagate all possible extension attributes (but it should if it can). The paragraph is not intended to address arbitrary HTTP headers that have nothing to do with CE or CE extensions (e.g. "User-Agent"). CE is not intended to be an HTTP proxy (especially because it is supposed to be transport-agnostic). So that said, I stand by my recommendation. We should not be processing arbitrary HTTP headers in the general case. If someone (not a Google service for whom we're explicitly converting legacy events) sends us a CloudEvent, and includes a User-Agent header, that User-Agent is not part of the cloud event, and should not be exposed to the user. Similarly, when converting legacy events (such as from Storage or PubSub), we should not be processing arbitrary HTTP headers. However, if there are specific headers that provide important data (such as trace correlation context), and fit into a transport-agnostic event paradigm (and preferably fit into an existing CE attribute or extension), then I think we can consider updating the legacy event conversion to bring that data in. |
I think what may be lost here is why this issue was opened in the first place. Adding context: This issue was opened as a response to this thread: googleapis/nodejs-logging#591 The primary use case is debugging. We want to be able to conveniently look at all logs related to the execution of one event. This can be done with http functions since they expose headers and we can look at the trace and execution id and add them to each log and now all the logs can easily be looked up via the trace and/or execution id. However with background functions, no trace/execution id is exposed. This makes it so we cannot connect all logs related to these function's executions and makes debugging a lot more painful. Having a way to easily debug high traffic cloud functions is essential, and this will definitely help a ton. Personally, I would have loved Cloud Functions to expose the trace/execution Id as part of the environment, regardless of background, http, firebase function. Here's the difference in logging with access to executionId and not having access (Please note in both instances I've only invoked the function once so I know exactly what logs below to which execution, but functions running in prod are being invoked 10s of times per second and all the logs are jumbled. |
OK, thanks for the comment @govindrai. It seems like we just need to expose these headers: req.get('function-execution-id');
req.get('x-cloud-trace-context'); The POC exposes them to the user via the
|
Hello, any progress on exporting these headers to background functions in the context? |
@jskeet What was the status on the spec for legacy event to cloud event conversion? It seems like we may need to convert and expose these two headers, if present. |
@dazuma: We still have a few things to iron out for some Firebase events. I'm not sure what to do in terms of these headers, but we should work that out. |
Hello, really looking for this feature. I'm fixing logging in a cloud function right now and implementing winston logger. I lose the execution_id by doing this and I'm trying to figure out how to get it back. |
Based on the paragraph above (sourced from this doc) and Daniel's comment above, it seems like extension attributes are indeed the best approach here. Moreover, there are already extension attributes for Distributed Tracing. However, I'm not sure if: |
GCF event requests send information in the HTTP headers that we currently don't expose to the user.
Example HTTP headers seen from a GCF V1 Pub/Sub event (here, represented as an object):
V2:
Other events besides Pub/Sub likely include some of these headers, or will in V2.
I'd assume that a CloudEvent signature would have something like:
Example HTTP headers:
I assume we can just expose all of these headers to the function's first parameter. A user can decide which ones they care about. A CloudEvent
There should be no clash of
cloudevent
as none of these identifiers: "id", "source", "specversion", "type", "datacontenttype", "dataschema", "subject", "time" are HTTP headers.We can also add this to the Event signature, though not really sure what interface we want.
(This raw HTTP request was captured using a custom Node Functions Framework)
The text was updated successfully, but these errors were encountered: