Closed Bug 1605097 Opened 8 months ago Closed 1 month ago

Persist HTTP headers for pending pings

Categories

(Data Platform and Tools :: Glean: SDK, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: brizental, Assigned: brizental)

References

Details

Attachments

(1 file)

From a comment in from [:Dexter] https://docs.google.com/document/d/1YEhrudNl5aHBnht3FCjT66Bj58ckDw3jO-_V56mnqHs/edit?usp=sharing:

The debug tag is one of the headers that need to be persisted (pings can fail to upload and we still want to send them to the debug endpoint next time) :) The date one, for example, doesn't need to be persisted (it's added on the fly when uploading)

Whiteboard: [telemetry:glean-rs:m?] → [telemetry:glean-rs:m16]
Blocks: 1648012
Assignee: nobody → brizental
Priority: P3 → P1

Currently there is only one header which we need to persist, the X-Debug-ID header.

Soon though there will be more headers that will need persisting, see Bug 1634064. Therefore it is best to create a system for persisting these headers that will be fit for persisting as many as necessary.

I propose adding an optional new line to the ping file contents, with a JSON object containing all the custom headers. In case the ping does not have any custom headers the third line is just not added to the file, thus removing the need for any kind of migration for current ping file format.

What do you think [:janerik], [:Dexter]?

Flags: needinfo?(jrediger)
Flags: needinfo?(alessio.placitelli)

(In reply to Beatriz Rizental from comment #1)

What do you think [:janerik], [:Dexter]?

I like the approach. Since we're here, we might as well include a general "metadata" section on the third line, to make it easy to extend in the future, if needed. For example, the JSON object could be something like.

{
  "persistedHeaders": [
    {
      "name": "X-Debug-ID",
      "value": ... 
    },
    ..
  ]
}

This way, if we'd ever want to add something more than persisted headers, we'd just add a new section to this jSON object.

Flags: needinfo?(alessio.placitelli)

Can we currently think of any other information we might need to add in the future?
If no, then it sounds fine to be, even if a bit adhoc.
If yes, we might want to reconsider the file format and use something less adhoc.

At the very least we should have explicit tests for these of course.

(In reply to Alessio Placitelli [:Dexter] from comment #2)

(In reply to Beatriz Rizental from comment #1)

What do you think [:janerik], [:Dexter]?

I like the approach. Since we're here, we might as well include a general "metadata" section on the third line, to make it easy to extend in the future, if needed. For example, the JSON object could be something like.

This way, if we'd ever want to add something more than persisted headers, we'd just add a new section to this jSON object.

If we do this let's not forget documenting the format properly (bikeshedding upcoming, I for one think we might not need the "name" / "value" part and the "persisted" is also superfluous)

Flags: needinfo?(jrediger)

(In reply to Jan-Erik Rediger [:janerik] from comment #4)

(In reply to Alessio Placitelli [:Dexter] from comment #2)

(In reply to Beatriz Rizental from comment #1)

What do you think [:janerik], [:Dexter]?

I like the approach. Since we're here, we might as well include a general "metadata" section on the third line, to make it easy to extend in the future, if needed. For example, the JSON object could be something like.

This way, if we'd ever want to add something more than persisted headers, we'd just add a new section to this jSON object.

If we do this let's not forget documenting the format properly (bikeshedding upcoming, I for one think we might not need the "name" / "value" part and the "persisted" is also superfluous)

Heheh, fair point, I'm fine with dropping the above and go with your recommendation ;)

Whiteboard: [telemetry:glean-rs:m16]
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.