-
Notifications
You must be signed in to change notification settings - Fork 524
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 support for Composite spans in the intake API #5661
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
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.
Nice work! What's there looks good, just a few very minor comments aside from the one about end
. I'm not sure if we really need that. If we do need it, I'm wondering if we could store it as the ECS field event.end
instead.
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.
Basically LGTM, but I just realised we still need to update https://github.com/elastic/apm-server/blob/master/model/span/_meta/fields.yml. This defines how fields are mapped in Elasticsearch. After that file is updated, you will need to run make update
to update generated docs and integration package files.
As you're aware @axw, the agent spec is still being discussed and updated. So I'm going to wait until that's settled a bit more, update this PR, then request a review again. Thanks for your review so far! |
This pull request is now in conflicts. Could you fix it @estolfo? 🙏
|
ce064c5
to
938f56d
Compare
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.
Looks great, thanks! Just one minor comment. Please go ahead and merge after adding a comment on Composite.Sum. When you do, please add the backport-7.x
label to have Mergify automatically create the backport.
…) (#5777) * Support composite spans in the intake API (cherry picked from commit d0310d1) * Update documentation for new composite span fields in ES (cherry picked from commit 93332d2) # Conflicts: # include/fields.go * Update approvals file (cherry picked from commit 598e573) * Update duration and composite sum fields to be more realistic (cherry picked from commit edaedcc) * Update changelog (cherry picked from commit 3429db6) # Conflicts: # changelogs/head.asciidoc * Fix minor typo in composite.sum field (cherry picked from commit b827b2b) # Conflicts: # include/fields.go * Update description of span duration and composite sum (cherry picked from commit 24284d8) * Add note to Composite.Sum that it's in milliseconds (cherry picked from commit f65e583) * Update include/fields.go * Remove head.asciidoc Co-authored-by: Emily Stolfo <emily.stolfo@elastic.co>
Confirmed with BC2 |
Motivation/summary
Support a composite object in the span model and in the intake API. The composite object holds metrics on a group of spans represented by a single one.
The composite object has the following format
Checklist
How to test these changes
To test these changes specifically, not including the changes to span metrics calculations:
make
docker compose up -d
apm-server.yml
(username: "admin"
,password: "changeme"
)./apm-server -c apm-server.yml -e -d "*"
curl -H "Content-type: application/x-ndjson" --data-binary @testdata/intake-v2/spans.ndjson http://localhost:8200/intake/v2/events
composite
nested object.Expected nested document:
This can also be tested along with #5595.
Related issues
Related #5485
Related #5595