-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
POC: Simplifying un-abstracting projections and setup #70157
POC: Simplifying un-abstracting projections and setup #70157
Conversation
|
||
const params = mergeProjection(projection, { | ||
const params = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving away from mergeProjection
will make the code a lot more clear - especially for reviewers when things are changed/moved around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why we have mergeProjection
is two-fold:
lodash.merge
merges arrays in really weird ways- if you change a projection, its consumers do not need to update anything (usually).
The downside of spreading only means that you can miss places where the projection is being used. E.g., you'd have to manually inspect all references. Maybe that's a good thing though. Again not a defense - just want to highlight why it's there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you change a projection, its consumers do not need to update anything (usually).
This makes sense. Could we achieve the same with unit tests to ensure that projections are correctly applied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that depends on the nature of the change. for additive changes, no. for other changes, yes (arguably already covered by snapshot testing of queries)
}, | ||
}) | ||
: filter, | ||
filter: [...projection.body.query.bool.filter, ...serviceNameFilter], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good example of the confusion I get from the projection:
- we have a higher order function
mergeProjection
that merges some stuff - then we have
projection.body.query.bool
which is spreaded ontobool
(is this needed if we already havemergeProjection
?) - then we have a specific part of the projection being merged with the full
bool
projection + some other stuff
What takes precendence? Is ...projection.body.query.bool
really needed?
In my rewrite I propose we do away with mergeProjection
because it's quite opaque in it's merge behaviour. Instead I suggest we do dead-simple spread concatenations (filter: [...projection.body.query.bool.filter, ...serviceNameFilter]
), or even prop assignments (index: projection.index
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mergeProjection
= mergePlainObject
, e.g. it doesn't merge arrays. If you merge arrays, you can't remove items. Arguably the same applies for merging objects, but that feels more natural. If I speak for myself, in some cases I use explicit spreading so that the shape of the request (ie the result of mergeProjection
) is more clear. (Not a argument for or against projections, just an explanation).
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack APM API integration tests (basic).x-pack/test/apm_api_integration/basic/tests/feature_controls·ts.APM specs (basic) apm feature controls APIs can be accessed by global_all userStandard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / X-Pack APM API integration tests (basic).x-pack/test/apm_api_integration/basic/tests/feature_controls·ts.APM specs (basic) apm feature controls APIs can be accessed by global_all userStandard Out
Stack Trace
Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/apm/server/lib/services.services queries fetches the service itemsStandard Out
Stack Trace
and 1 more failures, only showing the first 3. Build metrics
To update your PR or re-run it, just comment with: |
@sqren just my 2$, I like |
@dgieselaar It's just yet another thing that's opaque. For instance when |
Started with a single file where
setup
is no longer passed (individual params are passed instead).