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

model: enhancements to support transaction aggregations #3639

Merged
merged 4 commits into from
Apr 14, 2020

Conversation

axw
Copy link
Member

@axw axw commented Apr 9, 2020

Motivation/summary

There are a few smallish changes here bundled together, in support of transaction duration aggregations (#3485).

Summary:

  • export metadata.System.Hostname, so its value can be used in aggregation keys
  • add metricset.Transaction.{Result,Root} fields, to use in aggregation keys
  • add metricset.Sample.{Counts,Values} fields, for indexing histogram metrics
  • use non-pointer fields for metricset.Metricset and related types to simplify usage, and reduce allocations

Note that the added fields are only added to the model types, and not the intake API. That will come later, as part of #3195.

Checklist

  • I have signed the Contributor License Agreement.
  • My code follows the style guidelines of this project (run make check-full for static code checks and linting)
  • I have rebased my changes on top of the latest master branch
    - [ ] I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
    - [ ] I have updated CHANGELOG.asciidoc

How to test these changes

make test

Related issues

#3485

axw added 4 commits April 9, 2020 15:09
Eport the System.Metadata (was System.hostname) method,
which will be used for grouping in the upcoming histogram
aggregator.
Change model/metricset to use non-pointer fields,
and adapt modeldecoder to the changes.
Changes to support transaction duration histograms.

- Add transaction.{root,result} fields.
- Add support for histogram metrics. This is currently
  only supported in the model type, and not in the
  intake API, as we lack a nice means of dynamic field
  mapping for histograms.
@axw axw requested a review from a team April 9, 2020 08:10
i++
valueFieldName := fieldName("value")
for name, s := range input {
sampleObj, _ := s.(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you removing the error check here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's guaranteed to be that type, by virtue of having passed JSON Schema validation. Since I moved schema validation into the exported Decode methods, you can no longer get here without passing validation.


// Counts holds the bucket counts for histogram metrics.
//
// These numbers must be positive or zero.
Copy link
Contributor

Choose a reason for hiding this comment

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

(Where) do you plan to add checks for the restrictions mentioned in the comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't planning on adding any checks beyond what Elasticsearch does, I just added that as a reminder/documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

In case it's not clear: if/when we add support for histogram metrics coming from agents, we would validate these constraints in modeldecoder. In the immediate future these fields will only be set by the metrics computed by the server, so we'll be in full control of the data.

@axw axw merged commit 71a6cc1 into elastic:master Apr 14, 2020
@axw axw deleted the model-metricset-enhancements branch April 14, 2020 08:20
axw added a commit to axw/apm-server that referenced this pull request Apr 14, 2020
* model/metadata: export System.Hostname method

Export the System.Metadata (was System.hostname) method,
which will be used for grouping in the upcoming histogram
aggregator.

* model/metricset: non-pointer field types

Change model/metricset to use non-pointer fields,
and adapt modeldecoder to the changes.

* model/metricset: add new metricset fields

Changes to support transaction duration histograms.

- Add transaction.{root,result} fields.
- Add support for histogram metrics. This is currently
  only supported in the model type, and not in the
  intake API, as we lack a nice means of dynamic field
  mapping for histograms.

* beater/config: fix some 'go vet' errors
axw added a commit that referenced this pull request Apr 20, 2020
* model/metadata: export System.Hostname method

Export the System.Metadata (was System.hostname) method,
which will be used for grouping in the upcoming histogram
aggregator.

* model/metricset: non-pointer field types

Change model/metricset to use non-pointer fields,
and adapt modeldecoder to the changes.

* model/metricset: add new metricset fields

Changes to support transaction duration histograms.

- Add transaction.{root,result} fields.
- Add support for histogram metrics. This is currently
  only supported in the model type, and not in the
  intake API, as we lack a nice means of dynamic field
  mapping for histograms.

* beater/config: fix some 'go vet' errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants