Skip to content

util: log metamorphic constants only with --show-logs#75065

Closed
irfansharif wants to merge 1 commit intocockroachdb:masterfrom
irfansharif:220118.metamorphic-logging
Closed

util: log metamorphic constants only with --show-logs#75065
irfansharif wants to merge 1 commit intocockroachdb:masterfrom
irfansharif:220118.metamorphic-logging

Conversation

@irfansharif
Copy link
Copy Markdown
Contributor

Makes some test invocations less verbose.

Release note: None

Makes some test invocations less verbose.

Release note: None
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@irfansharif
Copy link
Copy Markdown
Contributor Author

(Bump.)

Copy link
Copy Markdown
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

I'm not necessarily opposed to this, but is there a particular place that this came up as a problem? I worry that this would make it harder to do post-hoc debugging of CI failures caused by metamorphic constants.

stevendanna added a commit to stevendanna/cockroach that referenced this pull request Feb 9, 2022
Commands such as `./dev test --verbose` might previously would produce
two log lines everytime it happens to have to call the genrule for
execgen targets:

```
INFO: From Executing genrule //pkg/sql/colexec/colexecwindow:gen-window-framer:
initialized metamorphic constant "span-reuse-rate" with value 10
```

This is because execgen has the tracing package in its dependency tree

```
> go list -f '{{ join .Deps "\n" }}' ./pkg/sql/colexec/execgen/ | grep tracing | head -1
github.com/cockroachdb/cockroach/pkg/util/tracing
```

and that package instantiates this metamorphic constant.

This PR sets COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true when
calling the generator so we don't get metamorphic constants enabled.

Something like cockroachdb#75065 would give us more complete coverage of commands
that might be generating noise.

Release note: None
@stevendanna
Copy link
Copy Markdown
Collaborator

Now that I'm using dev more, I definitely see more spam from these log lines. I was able to cut down on some of the spam with #76309 but perhaps something along the lines of what you have here is needed. I wonder, perhaps we could look at some environment variable so that we always get the metamorphic output in CI?

@irfansharif
Copy link
Copy Markdown
Contributor Author

Steven did it better in #76309.

@irfansharif irfansharif closed this Feb 9, 2022
@irfansharif irfansharif deleted the 220118.metamorphic-logging branch February 9, 2022 17:28
craig bot pushed a commit that referenced this pull request Feb 10, 2022
76210: util/tracing: reduce allocations in EnsureForkSpan r=andreimatei a=andreimatei

This patch pre-allocates capacity for the options slice in
EnsureForkSpan. Before, we were reallocating the slice storage
repeatedly, as it was growing from 0 to 1 and from 1 to 2 elements.

This patch also simplifies, and slighly speeds up, EnsureChildSpan. This
guy was using a sync.Pool for its options slice. That is not necessary,
since the slice storage does not escape.

In the benchmarks below, detached-child=false is EnsureChildSpan and
detached-child=true is EnsureForkSpan.

> make bench PKG=./pkg/util/tracing BENCHES=BenchmarkSpanCreation TESTFLAGS="-count=5 -cpu=1,2,4,8,16"
name                                  old time/op    new time/op    delta
SpanCreation/detached-child=false       1.70µs ± 1%    1.61µs ± 1%   -5.06%  (p=0.008 n=5+5)
SpanCreation/detached-child=false-2     1.17µs ± 7%    1.04µs ± 3%  -10.70%  (p=0.008 n=5+5)
SpanCreation/detached-child=false-4      958ns ± 2%     893ns ±10%   -6.76%  (p=0.008 n=5+5)
SpanCreation/detached-child=false-8     1.11µs ±38%    0.95µs ± 1%     ~     (p=0.310 n=5+5)
SpanCreation/detached-child=false-16     925ns ± 1%     919ns ± 1%     ~     (p=0.286 n=4+5)
SpanCreation/detached-child=true        2.27µs ± 2%    1.88µs ± 0%  -17.27%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-2      1.82µs ±10%    1.33µs ±22%  -26.81%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-4      1.29µs ± 2%    1.02µs ± 1%  -21.35%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-8      1.16µs ± 1%    0.91µs ± 5%  -21.56%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-16     1.14µs ± 2%    0.97µs ± 1%  -15.08%  (p=0.008 n=5+5)

name                                  old alloc/op   new alloc/op   delta
SpanCreation/detached-child=false         288B ± 0%      288B ± 0%     ~     (all equal)
SpanCreation/detached-child=false-2       288B ± 0%      288B ± 0%     ~     (all equal)
SpanCreation/detached-child=false-4       288B ± 0%      288B ± 0%     ~     (all equal)
SpanCreation/detached-child=false-8       288B ± 0%      288B ± 0%     ~     (all equal)
SpanCreation/detached-child=false-16      288B ± 0%      288B ± 0%     ~     (all equal)
SpanCreation/detached-child=true          768B ± 0%      288B ± 0%  -62.50%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-2        768B ± 0%      288B ± 0%  -62.50%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-4        768B ± 0%      288B ± 0%  -62.50%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-8        768B ± 0%      288B ± 0%  -62.50%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-16       769B ± 0%      288B ± 0%  -62.55%  (p=0.008 n=5+5)

name                                  old allocs/op  new allocs/op  delta
SpanCreation/detached-child=false         6.00 ± 0%      6.00 ± 0%     ~     (all equal)
SpanCreation/detached-child=false-2       6.00 ± 0%      6.00 ± 0%     ~     (all equal)
SpanCreation/detached-child=false-4       6.00 ± 0%      6.00 ± 0%     ~     (all equal)
SpanCreation/detached-child=false-8       6.00 ± 0%      6.00 ± 0%     ~     (all equal)
SpanCreation/detached-child=false-16      6.00 ± 0%      6.00 ± 0%     ~     (all equal)
SpanCreation/detached-child=true          16.0 ± 0%       6.0 ± 0%  -62.50%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-2        16.0 ± 0%       6.0 ± 0%  -62.50%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-4        16.0 ± 0%       6.0 ± 0%  -62.50%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-8        16.0 ± 0%       6.0 ± 0%  -62.50%  (p=0.008 n=5+5)
SpanCreation/detached-child=true-16       16.0 ± 0%       6.0 ± 0%  -62.50%  (p=0.008 n=5+5)

Release note: None

76308: changefeedccl: remove methods from testServerShim r=miretskiy a=stevendanna

In #75852 these methods have recently been added to the underlying
TestTenantInterface, so we no longer need to stub them out in this
shim.

Release note: None

76309: build: disable metamorphic constants when calling execgen r=irfansharif a=stevendanna

Commands such as `./dev test --verbose` might previously produce
two log lines everytime it happens to have to call the genrule for
execgen targets:

```
INFO: From Executing genrule //pkg/sql/colexec/colexecwindow:gen-window-framer:
initialized metamorphic constant "span-reuse-rate" with value 10
```

This is because execgen has the tracing package in its dependency tree

```
> go list -f '{{ join .Deps "\n" }}' ./pkg/sql/colexec/execgen/ | grep tracing | head -1
github.com/cockroachdb/cockroach/pkg/util/tracing
```

and that package instantiates this metamorphic constant.

This PR sets COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true when
calling the generator so we don't get metamorphic constants enabled.

Something like #75065 would give us more complete coverage of commands
that might be generating noise.

Release note: None

76338: jobs: De-flake TestTransientTxnErrors test. r=miretskiy a=miretskiy

Fix rare, but silly flake in TestTransientTxnErrors where the
`executeSchedules` go routine could run before we were ready.

Fixes #65045

Release Notes: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
RajivTS pushed a commit to RajivTS/cockroach that referenced this pull request Mar 6, 2022
Commands such as `./dev test --verbose` might previously would produce
two log lines everytime it happens to have to call the genrule for
execgen targets:

```
INFO: From Executing genrule //pkg/sql/colexec/colexecwindow:gen-window-framer:
initialized metamorphic constant "span-reuse-rate" with value 10
```

This is because execgen has the tracing package in its dependency tree

```
> go list -f '{{ join .Deps "\n" }}' ./pkg/sql/colexec/execgen/ | grep tracing | head -1
github.com/cockroachdb/cockroach/pkg/util/tracing
```

and that package instantiates this metamorphic constant.

This PR sets COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true when
calling the generator so we don't get metamorphic constants enabled.

Something like cockroachdb#75065 would give us more complete coverage of commands
that might be generating noise.

Release note: None
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.

3 participants