Skip to content
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

Mempool tracer without bang patterns #3244

Merged
merged 1 commit into from
Aug 30, 2021
Merged

Mempool tracer without bang patterns #3244

merged 1 commit into from
Aug 30, 2021

Conversation

jutaro
Copy link
Contributor

@jutaro jutaro commented Jul 5, 2021

For new tracing it is advisable/necessary to remove the strictness annotations of the tracer type.

  1. These objects are guaranteed to live only very short (with the new tracing framework).
  2. Their is a great probability, that these messages will be filtered out, in which case laziness is better.
  3. This is of all the dozens of tracer types the only one with such strictness annotations.
  4. The new tracing infrastructure works prototype based. Since it is difficult to construct these objects, we use 'undefined' to make it work, which is not possible with these strictness annotations.

@jutaro jutaro requested a review from nfrisby as a code owner July 5, 2021 14:52
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the nice PR description! LGTM.

@nfrisby
Copy link
Contributor

nfrisby commented Jul 6, 2021

(Edit: It looks like this worked; the Nix build is now green.)

It looks like you won the lottery with that Nix failure 👏👏👏

I have opened #3250 to look into it.

I anticipate that if we ran this PR's Hydra evals again, it wouldn't happen; I think you just got "lucky". So I'm going to try to force Hydra to do that now.

I restarted https://hydra.iohk.io/build/6835752, which had looked like this:

image

Your latest was https://hydra.iohk.io/build/6836123, which used a cached failure from the above. I figure Restarting that one would just use the cached failure again. So I think I restart 6835752 and then once it finishes restart 6836123 and then we'll be green 🤞

@EncodePanda
Copy link
Contributor

This PR is now green, should we merge it? @nfrisby @jutaro

@jutaro
Copy link
Contributor Author

jutaro commented Aug 17, 2021

Yes, please merge

@nfrisby
Copy link
Contributor

nfrisby commented Aug 17, 2021

@jutaro Would you rebase it? I expect that git fetch origin; git rebase -i origin/master will work without conflicts. Then we can invoke bors to merge it. Thanks!

@jutaro
Copy link
Contributor Author

jutaro commented Aug 17, 2021

I have done the rebasing

@nfrisby
Copy link
Contributor

nfrisby commented Aug 17, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 17, 2021
3244: Mempool tracer without bang patterns r=nfrisby a=jutaro

For new tracing it is advisable/necessary  to remove the strictness annotations of the tracer type.

1. These objects are guaranteed to live only very short (with the new tracing framework).
2. Their is a great probability, that these messages will be filtered out, in which case laziness is better.
3. This is of all the dozens of tracer types the only one with such strictness annotations.
4. The new tracing infrastructure works prototype based. Since it is difficult to construct these objects, we use 'undefined' to make it work, which is not possible with these strictness annotations.  

Co-authored-by: Yupanqui <jnf@arcor.de>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 17, 2021

Build failed:

@deepfire
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 30, 2021

@iohk-bors iohk-bors bot merged commit f03523f into master Aug 30, 2021
@iohk-bors iohk-bors bot deleted the jutaro/tracer-patch branch August 30, 2021 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants