feat: add ingress events to timeline #2735
Conversation
wesbillman
requested review from
a team and
deniseli
and removed request for
a team
wesbillman
force-pushed
the
add-ingress-events
branch
2 times, most recently
from
fd34982 to
11024ed
Compare
| } | ||
| observability.Ingress.Request(r.Context(), r.Method, r.URL.Path, optional.Some(verbRef), startTime, optional.Some(errMsg)) |
There was a problem hiding this comment.
Would you mind changing the final optional.Some back to sending a static string instead of err.Error()? Otherwise the cardinality of the error field blows up and makes brad cry tears of money
Same for all the other observability calls
There was a problem hiding this comment.
Will do! I was under the impression that attributes changing would not increase billing, but it sounds like that's not the case so thanks for catching this! I was mostly trying to avoid all the duplication here, but sounds that duplication is useful in this case.
I'll try to clean this up with some helper functions.
There was a problem hiding this comment.
Ahhh that makes sense. Thanks thanks!
| observability.Ingress.Request(r.Context(), r.Method, r.URL.Path, optional.Some(verbRef), startTime, optional.Some(err.Error())) | ||
| ingressEvent.Response = &http.Response{StatusCode: http.StatusInternalServerError} | ||
| ingressEvent.Error = optional.Some(err.Error()) | ||
| timelineService.InsertHTTPIngress(r.Context(), &ingressEvent) |
There was a problem hiding this comment.
What do you think about making a helper func for these, since there's the duplication between here and below?
There was a problem hiding this comment.
So much duplication 😂 helpers to the rescue
There was a problem hiding this comment.
The helper was gnarly to cover everything so I just made a helper for the new timeline error events. :)
| func callEventToCall(event *CallEvent) *Call { | ||
| var response either.Either[*ftlv1.CallResponse, error] | ||
| if eventErr, ok := event.Error.Get(); ok { | ||
| err := errors.New(eventErr) |
There was a problem hiding this comment.
Since this err is only used on the line below, what do you think about inlining it as response = either.RightOf[*ftlv1.CallResponse](errors.New(eventErr))?
| Request json.RawMessage `json:"request"` | ||
| RequestHeader json.RawMessage `json:"req_header"` | ||
| Response json.RawMessage `json:"response"` | ||
| ResponseHeader json.RawMessage `json:"res_header"` |
There was a problem hiding this comment.
ultra nitty nit: I don't think I've seen response abbreviated to res - only to resp. It is annoying that resp is 4 long while req is 3, so they don't match nicely then, but if I saw res_header with no context, I probably wouldn't guess what it is. What do you think? No strong feelings on my part
| Error optional.Option[string] `json:"error,omitempty"` | ||
| } | ||
|
|
||
| type Ingress struct { |
There was a problem hiding this comment.
Wanna nest this in IngressEvent?
There was a problem hiding this comment.
I very much do, but they are a bit too different. Like when creating an event I never want the requestKey to be missing, but it's allowed to be in the db. Similar to Request and Response where since they are different types it might get confusing to users of the API.
I had to do something similar with CallEvent :(
There was a problem hiding this comment.
ahhhh got it, that makes sense
| Request: []byte(`{}`), | ||
| RequestHeader: []byte("{}"), | ||
| Response: []byte(`{}`), | ||
| ResponseHeader: []byte("{}"), |
There was a problem hiding this comment.
Two things:
- Should we add another test with more content in these fields to make sure all the json marshaling works correctly?
- I feel like I'm probably missing something but why do some of these have " and others have `?
There was a problem hiding this comment.
- You bet!
- Nah, just copy/pasta errors :) Thanks for flagging!
There was a problem hiding this comment.
This was surprisingly hard to decipher the json marshalling 😂
| <ul className='pt-4 space-y-2'> | ||
| <li> |
There was a problem hiding this comment.
Do we actually need all these to be wrapped in the <ul> and <li> elements?
There was a problem hiding this comment.
It's consistent with the other detail pages for now, but yeah we should probably take a pass at these at some point. I'm not sure we even want to leave this current layout tbh.
| {selectedIngress.response !== 'null' && ( | ||
| <> | ||
| <div className='text-sm pt-2'>Response</div> | ||
| <CodeBlock code={JSON.stringify(JSON.parse(selectedIngress.response), null, 2)} language='json' /> | ||
| </> | ||
| )} | ||
|
|
||
| {selectedIngress.requestHeader !== 'null' && ( | ||
| <> | ||
| <div className='text-sm pt-2'>Request Header</div> | ||
| <CodeBlock code={JSON.stringify(JSON.parse(selectedIngress.requestHeader), null, 2)} language='json' /> | ||
| </> | ||
| )} | ||
|
|
||
| {selectedIngress.responseHeader !== 'null' && ( | ||
| <> | ||
| <div className='text-sm pt-2'>Response Header</div> | ||
| <CodeBlock code={JSON.stringify(JSON.parse(selectedIngress.responseHeader), null, 2)} language='json' /> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
Since these are so similar, what do you think about adding a helper component that looks like:
const CodeBlockIfNotNull({ heading, maybeNullContent }) => {
if (maybeNullContent === 'null') {
return
}
return (
<>
<div className='text-sm pt-2'>{heading}</div>
<CodeBlock code={JSON.stringify(JSON.parse(maybeNullContent), null, 2)} language='json' />
</>
)
}
There was a problem hiding this comment.
Same below for the attribute badges
There was a problem hiding this comment.
I made a helper for the code blocks that are just strings but we'll want to rethink all of this with some better design so I don't want to spend too much effort on it just yet :)
wesbillman
force-pushed
the
add-ingress-events
branch
from
11024ed to
004f7d1
Compare
Fixes #2518
Successful ingress request
Failed ingress request
