Skip to content

remove stacktrace.ContextSetter#1187

Merged
stuartnelson3 merged 4 commits intoelastic:masterfrom
stuartnelson3:remove-stacktrace-context-setter
Jan 12, 2022
Merged

remove stacktrace.ContextSetter#1187
stuartnelson3 merged 4 commits intoelastic:masterfrom
stuartnelson3:remove-stacktrace-context-setter

Conversation

@stuartnelson3
Copy link
Contributor

@stuartnelson3 stuartnelson3 commented Jan 11, 2022

closes #646
@stuartnelson3 stuartnelson3 requested a review from a team January 11, 2022 14:16
@ghost
Copy link

ghost commented Jan 11, 2022 •
edited by ghost
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

@axw axw Jan 12, 2022

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
logger WarningLogger
metricsGatherers []MetricsGatherer
contextSetter stacktrace.ContextSetter
preContext, postContext int
Copy link
Member

@axw axw Jan 12, 2022

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

@stuartnelson3 stuartnelson3 Jan 12, 2022

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

stuartnelson3 commented Jan 12, 2022

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