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

Add tracing to envtool tests run #3695

Merged
merged 34 commits into from Dec 6, 2023
Merged

Add tracing to envtool tests run #3695

merged 34 commits into from Dec 6, 2023

Conversation

hungaikev
Copy link
Contributor

@hungaikev hungaikev commented Nov 10, 2023

Description

Adding tracing to envtool tests run

Closes #3665

Readiness checklist

  • I added/updated unit tests (and they pass).
  • I added/updated integration/compatibility tests (and they pass).
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Milestone (Next), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@hungaikev hungaikev requested review from a team and AlekSi as code owners November 10, 2023 10:34
@CLAassistant
Copy link

CLAassistant commented Nov 10, 2023

CLA assistant check
All committers have signed the CLA.

@AlekSi AlekSi added the code/chore Code maintenance improvements label Nov 10, 2023
@AlekSi AlekSi added this to the Next milestone Nov 10, 2023
@AlekSi AlekSi self-assigned this Nov 10, 2023
@AlekSi AlekSi changed the title Add tracing to envtool tests run Add tracing to envtool tests run Nov 10, 2023
@AlekSi AlekSi enabled auto-merge (squash) November 10, 2023 10:41
Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #3695 (df271fa) into main (504c567) will decrease coverage by 0.47%.
The diff coverage is 95.65%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3695      +/-   ##
==========================================
- Coverage   32.31%   31.85%   -0.47%     
==========================================
  Files         315      316       +1     
  Lines       23583    23586       +3     
==========================================
- Hits         7622     7514     -108     
- Misses      15277    15393     +116     
+ Partials      684      679       -5     
Files Coverage Δ
internal/util/ctxutil/sigterm_unix.go 100.00% <100.00%> (ø)
internal/util/observability/observability.go 100.00% <100.00%> (ø)
internal/util/testutil/testutil.go 77.50% <100.00%> (ø)
integration/setup/startup.go 42.85% <50.00%> (-21.25%) ⬇️

... and 7 files with indirect coverage changes

Flag Coverage Δ
filter-true 5.19% <95.65%> (-0.77%) ⬇️
hana-1 ?
integration 5.19% <95.65%> (-0.77%) ⬇️
mongodb-1 5.19% <95.65%> (+0.01%) ⬆️
unit 30.77% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

I don't see where this PR adds spans to envtool tests run command implementation in envtool.go. This issue is about adding tracing to it, not to its tests.

Please also run task all (or at least task fmt lint) locally.

.gitignore Outdated Show resolved Hide resolved
cmd/envtool/envtool.go Outdated Show resolved Hide resolved
auto-merge was automatically disabled November 10, 2023 16:00

Head branch was pushed to by a user without write access

Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

That's not what needs to be done in that issue.

Let's use one trace per top-level test and spans for subtests

Not one span per go test invocation and one tracing event per decoded event. We need to start span when the decoded event indicates that the test started, end span when the test ends, handle paused and continued tests, etc.

go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

mergify bot commented Nov 15, 2023

@hungaikev this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label Nov 15, 2023
cmd/envtool/tests.go Outdated Show resolved Hide resolved
@mergify mergify bot removed the conflict PRs that have merge conflicts label Nov 28, 2023
@AlekSi
Copy link
Member

AlekSi commented Dec 4, 2023

@hungaikev any news? Anything we could help you with?

@AlekSi AlekSi modified the milestones: v1.16.0, Next Dec 4, 2023
@hungaikev
Copy link
Contributor Author

@hungaikev any news? Anything we could help you with?

Hi @AlekSi would you be willing or available for a <= 15-minute block for us to go through this together to reduce the back and forth? I have a feeling that the part left is very small but seems to be taking 80% of the effort.

@AlekSi
Copy link
Member

AlekSi commented Dec 6, 2023

I fixed all the problems. The result looks like this:

CleanShot 2023-12-06 at 18 57 19@2x

@AlekSi AlekSi enabled auto-merge (squash) December 6, 2023 14:58
@AlekSi AlekSi self-requested a review December 6, 2023 14:58
AlekSi
AlekSi previously approved these changes Dec 6, 2023
@AlekSi
Copy link
Member

AlekSi commented Dec 6, 2023

@rumyantseva PTAL

rumyantseva
rumyantseva previously approved these changes Dec 6, 2023
Copy link
Member

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

So, with the current approach, we can see when a particular event (run, continue, pause, pass) happened for each subtest. Looks good to me.

@AlekSi AlekSi dismissed stale reviews from rumyantseva and themself via df271fa December 6, 2023 17:19
@AlekSi AlekSi disabled auto-merge December 6, 2023 17:24
@AlekSi AlekSi merged commit f814189 into FerretDB:main Dec 6, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/chore Code maintenance improvements
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add tracing to envtool tests run
4 participants