feat(sessions): Basic session handling #178
Conversation
986cb91 to
26880de
Compare
26880de to
4445a0d
Compare
| size_t body_len = sentry__stringbuilder_len(&sb); | ||
| char *body = sentry__stringbuilder_into_string(&sb); | ||
| req = prepare_http_request(&event_id, endpoint_type, | ||
| "application/x-sentry-envelope", body, body_len, true); |
There was a problem hiding this comment.
hm, since we already require a sentry which can deal with envelopes, I was considering to massively simplify this (throwing out all the multipart handling), and basically make envelope 1 <-> 1 http request a 1:1 relation. but that is for the future.
There was a problem hiding this comment.
We can't do that unfortunately because of different rate limits.
There was a problem hiding this comment.
Just so I understand the intention correctly…
- In the normal case, we want to drop events from
capture_eventon rate-limit, right? - Also, with the changes to
capture_event, we attach the session to the captured envelope, and want to send it as different http requests for rate limiting purposes… - Then, on normal shutdown, we close the session and send it as its own envelope.
I wonder why we don’t just create a separate envelope for it?
Also since you mentioned raw envelopes, how should we proceed with that? I would personally prefer to still have a 1:1 relation between event->envelope->http request.
In any case, I think the rate limiting itself needs some work in the transports.
There was a problem hiding this comment.
You can actually throw everything into one request, since Sentry enforces the rate limits per item (or, at least it will eventually). It seems that with the above logic that removes rate limited items from envelopes, this should work just fine.
There is just one final issue remaining with a size limit on the /store/ endpoint, but we can solve this fairly easily.
|
|
||
| TEST_CHECK_INT_EQUAL(sentry__envelope_get_item_count(envelope), 1); | ||
|
|
||
| const sentry_envelope_item_t *item = sentry__envelope_get_item(envelope, 0); |
There was a problem hiding this comment.
maybe also add a second test to see what happens when you manually call capture_event…
|
Resolved most comments. |
| struct tm tm_buf; | ||
| tm = gmtime_r(&secs, &tm_buf); | ||
| #endif | ||
| size_t end = strftime(buf, sizeof buf, "%FT%T", tm); |
There was a problem hiding this comment.
@Amphaal So for some reason the mingw build returns 0 here, which means that the snprintf below overwrites the whole buffer, which in turn means that our tests are failing because of wrong time formatting here:
https://dev.azure.com/sentry-builds/sentry-native/_build/results?buildId=1789&view=logs&j=3e1f1b51-d184-5f5a-906a-d9e9ce13cd6b&t=372e633b-daac-5e1b-39f1-1bfa2728bd78
Do you have any quick idea about that?
I had a talk earlier with @jan-auer and given that none of us has the capacity to actually maintain this, and the problem that it just hangs and times out half of the builds on CI, and the builds generally being slower than all the other platforms.
We might just remove it from CI again, and demote it to kind of works, but we don’t actively maintain it status.
There was a problem hiding this comment.
Giving the context, it's fully understandable. The only way to reduce build time would be to use a precompiled docker image paired with the adequate CI (like Jenkins or CircleCI, I do not believe Azure Pipelines can do that rn). And that LLVM linker bug is really annoying... I'm 100% with you on this one.
There was a problem hiding this comment.
@Amphaal thanks for your contributions to this! In case you come around to find a good working solution, let us know and we can reenable builds again.
This doesn't do much yet but can become the start of proper session handling.
I believe we might want to persist session data to disk and read it back. I added some basic JSON parsing based on jsmn with some added ugly utf-8 handling.