Implement a sync marker

[sergey-miryanov]

Should implement #534

[sergey-miryanov]

@gaogaotiantian should I do something with failed build? It seems not related to my changes.

[gaogaotiantian]

No, it's failing all the CIs. I'll investigate that first.

[gaogaotiantian]

This is related to tonistiigi/binfmt#215

[gaogaotiantian]

It's a docker image issue and fixed in #543. You can rebase or merge your branch so this will go away.

[gaogaotiantian]

I took a quick look first. The overall approach is good, before I do a thorough review, just some direction suggestions.

I'm hoping that this feature touches less unnecessary code. You refactored some code - adding utilities and stuff, that might clean up the code a bit, but I think if it should be done, it should be done in a separate PR. Let's just do this sync marker feature as isolated as possible. Which means, probably just a set_sync_marker and a get_sync_marker method added for the C tracer. (And probably report a warning or something if the user tries to set sync marker twice, that normally means a mistake).

For alignment, I don't think we need an extra argument --use_sync_marker. I probably mentioned this, but no sync marker should be equivalent to set sync marker on the very first trace event - the result will not be off too much. So when we align reports, if we can find one, use that, otherwise do the original stuff, which is exactly equivalent to sync marker at first event. The user does not need to use an extra option to load sync marker - it should just work.

The testing is thorough, which is great, but maybe reduce the complexity a bit. First of all, we don't need to add extra tests for cases where sync markers are not used at all. Negative tests are sometimes helpful, but that's a lot of code to test when a feature is not enabled.

For the temporary files, I know there are different ways to deal with them in the code base, but most of them are artifacts from when I was new to the project :) I think the better way is to create a temporary directory with tempfile.TemporaryDirectory and use that as a context. Put everything there (except maybe the final result.json file which is overwhelmingly used among tests) and python will clean up the tempdir for you.

This is not a huge feature but not trivial either, don't feel rushed and let's land it properly :)

[sergey-miryanov]

@gaogaotiantian Thanks for your review. Will make proposed changes.

[sergey-miryanov]

@gaogaotiantian Please take a look. Tests still a bit complicated, maybe you can suggest some simplifications.

[sergey-miryanov]

I believe I have couple ideas.

[gaogaotiantian]

I think for implementation part you did great - just a few comments. The test case could be simplified a bit :)

[sergey-miryanov]

Thank you for your review, will dig further :)

[sergey-miryanov]

@gaogaotiantian build 3.12 on ubuntu-last failed with Failed to CreateArtifact: Failed to make request after 5 attempts: Request timeout: /twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact. Should I do something with this?

[gaogaotiantian]

@gaogaotiantian build 3.12 on ubuntu-last failed with Failed to CreateArtifact: Failed to make request after 5 attempts: Request timeout: /twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact. Should I do something with this?

I don't believe so, I'll just restart the CI.

[gaogaotiantian]

Ahh and I forgot one thing - could you add some basic description in "combine reports" section of concurrency.rst? Just the usage of set_sync_marker(). Thanks!

[sergey-miryanov]

Ahh and I forgot one thing - could you add some basic description in "combine reports" section of concurrency.rst? Just the usage of set_sync_marker(). Thanks!

This is definitely the hardest part!

I took part of the basic_usage.rst and added lines about sync-marker. I'm not a native speaker, so I'm not sure I can properly describe the desired actions in the docs.

[gaogaotiantian]

Sorry that was my fault - it should not be in concurrency.rst. Just put the sync marker description after align_combine in basic usage. I forgot the existence of that piece :) Your description works - I'm not a native speaker either.

[sergey-miryanov]

Please take a look.

[gaogaotiantian]

Thank you for all the efforts! Will merge this soon.

[sergey-miryanov]

Thanks for your patience!