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

Add support for Composite spans in the intake API #5661

Merged
merged 8 commits into from
Jul 21, 2021

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Jul 9, 2021

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

{
   "name": "SELECT FROM product_types WHERE user_id=?",
   "timestamp": 1613575086000000,
   "duration": 3781,
   "composite": {
       	"count": 10,
        "sum": 3592,
        "compression_strategy": "exact_match"
    }
}

Checklist

How to test these changes

To test these changes specifically, not including the changes to span metrics calculations:

  • make
  • docker compose up -d
  • Add the ES credentials to 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
  • In Kibana Dev Tools, run the following query and verify the span document has the expected composite nested object.
GET /apm-8.0.0-span/_search
{
  "query": {
    "match": {
      "trace.id": "edcbaf0123456789abcdef9876543210"
    }
  }
}

Expected nested document:

            "composite" : {
              "count" : 10,
              "sum" : {
                "us" : 359298
              },
              "compression_strategy" : "exact_match"
            }

This can also be tested along with #5595.

Related issues

Related #5485
Related #5595

@apmmachine
Copy link
Contributor

apmmachine commented Jul 9, 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-21T13:25:54.754+0000

  • Duration: 39 min 55 sec

  • Commit: f65e583

Test stats 🧪

Test Results
Failed 0
Passed 5965
Skipped 14
Total 5979

Trends 🧪

Image of Build Times

Image of Tests

@estolfo estolfo added the v7.15.0 label Jul 9, 2021
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.

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.

changelogs/7.15.asciidoc Outdated Show resolved Hide resolved
model/span.go Outdated Show resolved Hide resolved
model/modeldecoder/v2/model.go Outdated Show resolved Hide resolved
model/span.go Outdated Show resolved Hide resolved
model/span.go Outdated Show resolved Hide resolved
model/span.go Outdated Show resolved Hide resolved
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.

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.

@estolfo
Copy link
Contributor Author

estolfo commented Jul 13, 2021

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!

@mergify
Copy link
Contributor

mergify bot commented Jul 14, 2021

This pull request is now in conflicts. Could you fix it @estolfo? 🙏
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 composite-spans upstream/composite-spans
git merge upstream/master
git push upstream composite-spans

@estolfo estolfo marked this pull request as draft July 19, 2021 17:47
@estolfo estolfo force-pushed the composite-spans branch 2 times, most recently from ce064c5 to 938f56d Compare July 20, 2021 15:26
@estolfo estolfo marked this pull request as ready for review July 20, 2021 15:29
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.

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.

model/span.go Outdated Show resolved Hide resolved
@estolfo estolfo merged commit f831d2a into elastic:master Jul 21, 2021
axw pushed a commit that referenced this pull request Jul 22, 2021
…) (#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>
@stuartnelson3
Copy link
Contributor

Confirmed with BC2

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.

5 participants