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.

Sign up for GitHub

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

Jump to bottom

remove stacktrace.ContextSetter #1187

Merged
merged 4 commits into from
Jan 12, 2022
Merged

remove stacktrace.ContextSetter #1187

merged 4 commits into from
Jan 12, 2022

Conversation

stuartnelson3
Copy link
Contributor

closes #646

@stuartnelson3 stuartnelson3 requested a review from a team January 11, 2022 14:16
@apmmachine
Copy link
Contributor

apmmachine commented Jan 11, 2022 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-12T10:23:41.721+0000

  • Duration: 33 min 2 sec

  • Commit: 101a0c9

Test stats 🧪

Test Results
Failed 0
Passed 12089
Skipped 279
Total 12368

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

axw
axw reviewed Jan 12, 2022
View reviewed changes
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. I think there might be a few more related things we can get rid of.

modelwriter.go
if w.cfg.logger != nil {
w.cfg.logger.Debugf("setting context failed: %v", err)
}
w.stats.Errors.SetContext++
Copy link
Member

Choose a reason for hiding this comment

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

can we get rid of the SetContext field now?

tracer.go Outdated
@@ -525,7 +524,6 @@ type tracerConfig struct {
metricsInterval time.Duration
logger WarningLogger
metricsGatherers []MetricsGatherer
contextSetter stacktrace.ContextSetter
preContext, postContext int
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need preContext/postContext? I think we can get rid of all related config and env vars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like it, removed 🪄

axw
axw approved these changes Jan 12, 2022
View reviewed changes
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

🎉 LGTM!

@stuartnelson3
Copy link
Contributor Author

I didn't see any additional environment variables, but I cleaned up the pre/post context stuff, and code related to Errors.SetContext

@stuartnelson3 stuartnelson3 merged commit 4f4d32c into elastic:master Jan 12, 2022
@stuartnelson3 stuartnelson3 deleted the remove-stacktrace-context-setter branch January 12, 2022 12:13
@simitt
Copy link
Contributor

simitt commented Feb 9, 2022

@axw anything you think we should test for this? Nothing comes to my mind, so I'd skip manual testing on this one.

@axw
Copy link
Member

axw commented Feb 10, 2022

Nope, nothing to test here, we just took away an API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axw axw axw approved these changes

Assignees
No one assigned
Labels
agent-go breaking change test-plan-skip
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove stacktrace.ContextSetter
4 participants
@stuartnelson3 @apmmachine @simitt @axw