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

[Azure] [Container_instance] Add dimensions and metric_type metadata for the container_instance data_stream #7127

Conversation

tetianakravchenko
Copy link
Contributor

@tetianakravchenko tetianakravchenko commented Jul 24, 2023

What does this PR do?

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.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

…ta_stream

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@tetianakravchenko tetianakravchenko requested a review from a team as a code owner July 24, 2023 14:42
@tetianakravchenko
Copy link
Contributor Author

@zmoog I couldn't find those fields - https://github.com/elastic/integrations/blob/main/packages/azure_metrics/data_stream/container_instance/fields/fields.yml#L1

could you please check if this field is not added by mistake?

@elasticmachine
Copy link

elasticmachine commented Jul 24, 2023

💚 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: 2023-07-27T08:13:34.635+0000

  • Duration: 15 min 8 sec

Test stats 🧪

Test Results
Failed 0
Passed 134
Skipped 0
Total 134

🤖 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
Copy link

elasticmachine commented Jul 24, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (3/3) 💚
Files 100.0% (3/3) 💚 2.831
Classes 100.0% (3/3) 💚 2.831
Methods 73.333% (22/30) 👎 -18.854
Lines 100.0% (21/21) 💚 9.022
Conditionals 100.0% (0/0) 💚

@zmoog
Copy link
Contributor

zmoog commented Jul 25, 2023

could you please check if this field is not added by mistake?

Yep, I'll check it out!

changes:
- description: Add dimension and metric_type metadata to the container_instance data_stream
type: enhancement
link: https://github.com/elastic/integrations/pull/5123
Copy link
Contributor

Choose a reason for hiding this comment

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

Please correct the PR link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done - 7a77aa7

@@ -1,3 +1,8 @@
- version: "1.0.17"
changes:
- description: Add dimension and metric_type metadata to the container_instance data_stream
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- description: Add dimension and metric_type metadata to the container_instance data_stream
- description: Add dimension and metric_type metadata to the container_instance datastream

@@ -1,6 +1,6 @@
name: azure_metrics
title: Azure Resource Metrics
version: 1.0.16
version: 1.0.17
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not 1.1.0 ? Any specific reason ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no specific reason, I did it similarly to the AWS package, also I've noticed that previous changes in this package were also added with 1.0.X

Copy link
Contributor

Choose a reason for hiding this comment

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

Enabling TSDB seems worth a bump to the minor version, but I have no strong opinions. I'll leave the decision to you, TSDB folks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zmoog there will be another PR to enable TSDB by default (similar to this PR and additionally adding stack restriction), this PR just adds metadata to the fields, that will be used by TSDB

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
@mlunadia
Copy link

@bturquet can you help us getting these reviewed? We have a long queue of Azure packages waiting on it. Merci

@@ -1,6 +1,6 @@
name: azure_metrics
title: Azure Resource Metrics
version: 1.0.16
version: 1.0.17
Copy link
Contributor

Choose a reason for hiding this comment

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

Enabling TSDB seems worth a bump to the minor version, but I have no strong opinions. I'll leave the decision to you, TSDB folks.

@tetianakravchenko tetianakravchenko merged commit 1ca98fb into elastic:main Aug 2, 2023
4 checks passed
@tetianakravchenko tetianakravchenko deleted the azure-container-instance-tsdb branch August 2, 2023 08:00
@elasticmachine
Copy link

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

gizas pushed a commit that referenced this pull request Sep 5, 2023
…for the container_instance data_stream (#7127)

* add dimensions and metric_type metadata for the container_instance data_stream

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

* address review comments

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

* add agent.id field

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

* extend azure.container_instance field

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

---------

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>
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.

None yet

6 participants