[AWS][Kinesis] Add dimension fields for TSDB support by constanca-m · Pull Request #5891 · elastic/integrations

constanca-m · 2023-04-17T07:21:41Z

What does this PR do?

Add dimension fields to Kinesis datastream.

Details

To uniquely identify a Kinesis stream, we need the combination of stream name (unique per AWS region) + account ID + account region. There are no metrics split by labels, so no more dimensions should be needed. The tests with TSDB enabled and disabled did not show a change on the number of documents received.

Checklist

I have reviewed tips for building integrations and this pull request is aligned with them.
I have verified that all data streams collect metrics or logs.
I have added an entry to my package's changelog.yml file.
I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

Refer to #5864

Related issues

Relates to [AWS Kinesis] TSDB Enablement #5864

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

elasticmachine · 2023-04-17T08:11:20Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-05-04T07:06:25.496+0000
Duration: 52 min 53 sec

Test stats 🧪

Test	Results
Failed	0
Passed	188
Skipped	4
Total	192

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elasticmachine · 2023-04-17T08:11:30Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`15/15`)	💚
Files	93.75% (`15/16`)	👎 -3.421
Classes	93.75% (`15/16`)	👎 -3.421
Methods	86.131% (`236/274`)	👎 -6.502
Lines	85.925% (`7387/8597`)	👎 -6.45
Conditionals	100.0% (`0/0`)	💚

agithomas

Review feedback shared.

agithomas · 2023-04-18T00:43:11Z

packages/aws/data_stream/kinesis/fields/fields.yml

      type: group
      fields:
        - name: StreamName
+          dimension: true


Please add here the reason for adding the specific field as a dimension field.Adding the reason is among the best practices for TSDB enablement.

By "here" you mean in the manifest @agithomas? It's explained in "Details" in the PR description.

It can be added as the inline comment. Reference

I have certain thoughts around a better way to handle aws dimensions. We have a length limitation in dimension and AWS permits 30 dimensions

If all 30 names and values are fully used to max limit, the 32KB dimension field length limitation would reach.

Can we have fingerprint processor applied on all aws dimensions and use the new field (having fingerprint) used as a dimension field?

cc @tetianakravchenko , @lalit-satapathy

Ok, I will try to add it to the document without being too confused. I don't understand the other part though, there are only 3 fields set as dimension, why would we have the need for a processor? @agithomas

Please validate the proposal against

how often aws change the dimensions of a managed service

feasibility of including ingest pipeline and inclusion of new field only for implementing TSDB.

Sorry, I don't understand: is there a reason to create a new dimension using the dimensions.* field? The 3 fields set to dimension right now should be enough @agithomas

The advantages are mentioned here.

The above proposal is based only on the convenience of TSDB. Please compare and choose the best approach.

@agithomas from my understanding, aws dimensions shouldn't be a concern at least for this data_stream, since we set StreamName as a dimension (dimension in TSDB scope) - here is an sample of event.
So we set aws.dimensions.StreamName (the field of type keyword) , not the aws.dimensions.* (the field of type object)

tetianakravchenko · 2023-04-27T07:25:12Z

packages/aws/data_stream/kinesis/fields/ecs.yml

  name: cloud
 - external: ecs
  name: cloud.account.id
+  dimension: true


here was a suggestion to align on list of fields - elastic/ecs#2172 and was suggested to use cloud.project.id. Is this field available for AWS?

I haven't checked, but if we added that as a dimension it would be redundant, as we don't really need it set as a dimension @tetianakravchenko

we checked with @agithomas that cloud.project.id does not exist for all cloud providers, it only present for gcp.

tetianakravchenko · 2023-04-27T08:32:00Z

packages/aws/data_stream/kinesis/fields/fields.yml

      type: group
      fields:
        - name: StreamName
+          dimension: true


@constanca-m how this name is defined? as I see this name is not set in configuration

what if 2 kinesis data_stream in the same region will be created?

When we connect to the AWS, it fetches data from the existent data streams. We don't create any data stream when we add the integration. The stream name is unique per region, and since region is a dimension, it shouldn't be a problem @tetianakravchenko

tetianakravchenko

As agreed: it might be needed to add agent.id field as a dimension in future, for now keeping as is.

agithomas · 2023-05-02T16:08:13Z

@constanca-m , please confirm if the below tests are conducted ?

Verification and validation

Verification of data in visualisation after enabling TSDB flag in kibana
Verification of the count of documents (before & after TSDB enablement) in Discover Interface
Verify if field mapping is correct in the data stream template.

constanca-m · 2023-05-02T16:09:42Z

@constanca-m , please confirm if the below tests are conducted ?

Verification and validation

* [ ]  Verification of data in visualisation after enabling TSDB flag in kibana

* [ ]  Verification of the count of documents (before & after TSDB enablement) in Discover Interface

* [ ]  Verify if field mapping is correct in the data stream template.

Yes, for dimensions they are correct. I didn't check the tasks because they are influenced by the two PRs, one for dimensions and one for metrics. @agithomas

agithomas · 2023-05-02T16:20:50Z

@constanca-m , please confirm if the below tests are conducted ?

Verification and validation
* [ ]  Verification of data in visualisation after enabling TSDB flag in kibana

* [ ]  Verification of the count of documents (before & after TSDB enablement) in Discover Interface

* [ ]  Verify if field mapping is correct in the data stream template.
Yes, for dimensions they are correct. I didn't check the tasks because they are influenced by the two PRs, one for dimensions and one for metrics. @agithomas

Ok, Thanks. Please include this validation for metric_type mapping PR.

agithomas

LGTM!

elasticmachine · 2023-05-04T08:31:21Z

Package aws - 1.34.2 containing this change is available at https://epr.elastic.co/search?package=aws

Add dimension fields.

a90708f

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

constanca-m added enhancement New feature or request Integration:aws AWS labels Apr 17, 2023

constanca-m requested review from agithomas, lalit-satapathy, ritalwar and tetianakravchenko April 17, 2023 07:21

constanca-m self-assigned this Apr 17, 2023

constanca-m requested a review from a team as a code owner April 17, 2023 07:21

Update changelog.

3e54608

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

constanca-m mentioned this pull request Apr 17, 2023

[AWS Kinesis] TSDB Enablement #5864

Closed

5 tasks

agithomas reviewed Apr 25, 2023

View reviewed changes

tetianakravchenko reviewed Apr 27, 2023

View reviewed changes

tetianakravchenko approved these changes May 2, 2023

View reviewed changes

agithomas approved these changes May 3, 2023

View reviewed changes

kaiyan-sheng approved these changes May 3, 2023

View reviewed changes

Merge branch 'main' into tsdb-dimensions-kinesis

6577933

constanca-m merged commit a904bba into elastic:main May 4, 2023

constanca-m deleted the tsdb-dimensions-kinesis branch May 4, 2023 08:09

Conversation

constanca-m commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Details

Checklist

How to test this PR locally

Related issues

Uh oh!

elasticmachine commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

elasticmachine commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

agithomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constanca-m Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tetianakravchenko left a comment

Choose a reason for hiding this comment

Uh oh!

agithomas commented May 2, 2023

Verification and validation

Uh oh!

constanca-m commented May 2, 2023

Verification and validation

Uh oh!

agithomas commented May 2, 2023

Verification and validation

Uh oh!

agithomas left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented May 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

constanca-m commented Apr 17, 2023 •

edited

Loading

elasticmachine commented Apr 17, 2023 •

edited

Loading

elasticmachine commented Apr 17, 2023 •

edited

Loading

constanca-m Apr 26, 2023 •

edited

Loading