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

Move more code from model to model processors #5687

Merged
merged 2 commits into from
Jul 15, 2021

Conversation

axw
Copy link
Member

@axw axw commented Jul 13, 2021

Motivation/summary

Move more logic out of model transformation, into dedicated model processors:

  • modelprocessor.SetExcludeFromGrouping matches stack frames against a regular expression, setting the "exclude_from_grouping" field
  • modelprocessor.SetLibraryFrame matches stack frames against a regular expression, setting the "library_frame" field
  • modelprocessor.SetCulprit identifies the first source mapped, non-library, stack frame and uses it to set or update the error culprit

The "library_frame" field is now only set if it is true, to simplify code and keep documents minimal. The UI collapses stack frames identified as library frames, but takes omission to mean the same thing as false, i.e. "falsy", so this change has no other practical effects.

The new model processors are only installed for the RUM intake APIs. With these changes, we no longer need to identify events as originating from the RUM endpoint, so we remove the model.Span.RUM and model.Error.RUM fields.

How to test these changes

Send an exception event to APM Server, check that (non-)library frames are still differentiated.

Related issues

Pulling logic out of model transformation for

@apmmachine
Copy link
Contributor

apmmachine commented Jul 13, 2021

💚 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: 2021-07-14T08:04:23.113+0000

  • Duration: 39 min 46 sec

  • Commit: 74403cd

Test stats 🧪

Test Results
Failed 0
Passed 5925
Skipped 14
Total 5939

Trends 🧪

Image of Build Times

Image of Tests

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

Changes generally look good!

@axw axw force-pushed the modelprocessor-frames branch 2 times, most recently from a5d9bb9 to 292f301 Compare July 14, 2021 01:12
@axw axw marked this pull request as ready for review July 14, 2021 01:12
Move more logic out of model transformation,
into dedicated model processors:

 - modelprocessor.SetExcludeFromGrouping matches
   stack frames against a regular expression,
   setting the "exclude_from_grouping" field

 - modelprocessor.SetLibraryFrame matches stack
   frames against a regular expression, setting the
   "library_frame" field

 - modelprocessor.SetCulprit identifies the first
   source mapped, non-library, stack frame and uses
   it to set or update the error culprit

The "library_frame" field is now only set if it is
true. The UI uses this field but takes omission to
mean the same thing as false (i.e. "falsy")

The new model processors are only installed for
the RUM intake APIs. With these changes, we no
longer need to identify events as originating from
the RUM endpoint, so we remove the model.Span.RUM
and model.Error.RUM fields.
@axw axw force-pushed the modelprocessor-frames branch from 292f301 to 5afe6fb Compare July 14, 2021 02:44
@axw
Copy link
Member Author

axw commented Jul 14, 2021

/test

1 similar comment
@axw
Copy link
Member Author

axw commented Jul 14, 2021

/test

@mergify
Copy link
Contributor

mergify bot commented Jul 14, 2021

This pull request is now in conflicts. Could you fix it @axw? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b modelprocessor-frames upstream/modelprocessor-frames
git merge upstream/master
git push upstream modelprocessor-frames

@axw axw requested a review from simitt July 14, 2021 09:38
@axw axw merged commit ce483db into elastic:master Jul 15, 2021
@axw axw deleted the modelprocessor-frames branch July 15, 2021 05:12
mergify bot pushed a commit that referenced this pull request Jul 15, 2021
Move more logic out of model transformation,
into dedicated model processors:

 - modelprocessor.SetExcludeFromGrouping matches
   stack frames against a regular expression,
   setting the "exclude_from_grouping" field

 - modelprocessor.SetLibraryFrame matches stack
   frames against a regular expression, setting the
   "library_frame" field

 - modelprocessor.SetCulprit identifies the first
   source mapped, non-library, stack frame and uses
   it to set or update the error culprit

The "library_frame" field is now only set if it is
true. The UI uses this field but takes omission to
mean the same thing as false (i.e. "falsy")

The new model processors are only installed for
the RUM intake APIs. With these changes, we no
longer need to identify events as originating from
the RUM endpoint, so we remove the model.Span.RUM
and model.Error.RUM fields.

(cherry picked from commit ce483db)
axw added a commit that referenced this pull request Jul 15, 2021
Move more logic out of model transformation,
into dedicated model processors:

 - modelprocessor.SetExcludeFromGrouping matches
   stack frames against a regular expression,
   setting the "exclude_from_grouping" field

 - modelprocessor.SetLibraryFrame matches stack
   frames against a regular expression, setting the
   "library_frame" field

 - modelprocessor.SetCulprit identifies the first
   source mapped, non-library, stack frame and uses
   it to set or update the error culprit

The "library_frame" field is now only set if it is
true. The UI uses this field but takes omission to
mean the same thing as false (i.e. "falsy")

The new model processors are only installed for
the RUM intake APIs. With these changes, we no
longer need to identify events as originating from
the RUM endpoint, so we remove the model.Span.RUM
and model.Error.RUM fields.

(cherry picked from commit ce483db)

Co-authored-by: Andrew Wilkins <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@stuartnelson3 stuartnelson3 self-assigned this Aug 26, 2021
@stuartnelson3
Copy link
Contributor

Confirmed with BC2

@stuartnelson3 stuartnelson3 removed their assignment Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants