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

Group same timestamp metrics values in app_insights metricset #20403

Merged
merged 5 commits into from
Aug 4, 2020

Conversation

narph
Copy link
Contributor

@narph narph commented Aug 3, 2020

What does this PR do?

  • groups same timestamp metric values
  • removes any nil values

Why is it important?

  • groups same timestamp metric values
  • removes any nil values
  • existing test should cover changes

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 3, 2020
@narph narph self-assigned this Aug 3, 2020
@narph narph added [zube]: In Review Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team labels Aug 3, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 3, 2020
@narph narph added the needs_backport PR is waiting to be backported to other branches. label Aug 3, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 3, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #20403 updated]

  • Start Time: 2020-08-03T17:15:26.059+0000

  • Duration: 39 min 50 sec

Test stats 🧪

Test Results
Failed 0
Passed 824
Skipped 84
Total 908

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

LGTM, added a couple of small suggestions.

Comment on lines +28 to +29
groupedAddProp[fmt.Sprintf("%sTO%s", item.Body.Value.Start, item.Body.Value.End)] =
append(groupedAddProp[fmt.Sprintf("%sTO%s", item.Body.Value.Start, item.Body.Value.End)], *item.Body.Value)
Copy link
Member

Choose a reason for hiding this comment

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

Consider calculating the key in a single place to avoid mistakes.

Suggested change
groupedAddProp[fmt.Sprintf("%sTO%s", item.Body.Value.Start, item.Body.Value.End)] =
append(groupedAddProp[fmt.Sprintf("%sTO%s", item.Body.Value.Start, item.Body.Value.End)], *item.Body.Value)
key := fmt.Sprintf("%sTO%s", item.Body.Value.Start, item.Body.Value.End)
groupedAddProp[key] = append(groupedAddProp[key], *item.Body.Value)

@@ -66,7 +103,9 @@ func getMetric(addProp map[string]interface{}) map[string]interface{} {
switch val.(type) {
case map[string]interface{}:
for subKey, subVal := range val.(map[string]interface{}) {
metricNames[cleanMetricNames(fmt.Sprintf("%s.%s", key, subKey))] = subVal
if subVal != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Would the removal of nil values need another changelog entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think so, this is normal behavior, the app insights have no data for these metrics so no data will be sent.

@narph narph merged commit b356494 into elastic:master Aug 4, 2020
@narph narph deleted the insights-return branch August 4, 2020 08:04
narph added a commit to narph/beats that referenced this pull request Aug 4, 2020
…c#20403)

* mofidy doc

* work on data

* changelog

* mage fmt update

(cherry picked from commit b356494)
narph added a commit to narph/beats that referenced this pull request Aug 4, 2020
…c#20403)

* mofidy doc

* work on data

* changelog

* mage fmt update

(cherry picked from commit b356494)
narph added a commit that referenced this pull request Aug 4, 2020
#20418)

* mofidy doc

* work on data

* changelog

* mage fmt update

(cherry picked from commit b356494)
narph added a commit that referenced this pull request Aug 4, 2020
#20419)

* mofidy doc

* work on data

* changelog

* mage fmt update

(cherry picked from commit b356494)
v1v added a commit to v1v/beats that referenced this pull request Aug 6, 2020
…ne-2.0

* upstream/master:
  [docs] Promote ingest management to beta (elastic#20295)
  Upgrade elasticsearch client library used in tests (elastic#20405)
  Disable logging when pulling on python integration tests (elastic#20397)
  Remove pillow from testing requirements.txt (elastic#20407)
  [Filebeat][ATP Module]Setting user agent field required by the API (elastic#20440)
  [Ingest Manager] Send datastreams fields (elastic#20402)
  Add event.ingested to all Filebeat modules (elastic#20386)
  [Elastic Agent] Fix agent control socket path to always be less than 107 characters (elastic#20426)
  Improve cgroup_regex docs with examples (elastic#20425)
  Makes `metrics` config option required in app_insights (elastic#20406)
  Ensure install scripts only install if needed (elastic#20349)
  Update container name for the azure filesets (elastic#19899)
  Group same timestamp metrics values in app_insights metricset (elastic#20403)
  add_process_metadata processor adds container id even if process metadata not accessible (elastic#19767)
  Support "cluster" scope in Metricbeat elasticsearch module (elastic#18547)
  [Filebeat][SophosXG Module] Renaming module and fileset (elastic#20396)
  Update Suricata dashboards (elastic#20394)
  [Elastic Agent] Improve version, restart, enroll CLI commands (elastic#20359)
  Prepare home directories for docker images in a different stage (elastic#20356)
v1v added a commit to v1v/beats that referenced this pull request Aug 6, 2020
…allation

* upstream/master: (23 commits)
  [docs] Promote ingest management to beta (elastic#20295)
  Upgrade elasticsearch client library used in tests (elastic#20405)
  Disable logging when pulling on python integration tests (elastic#20397)
  Remove pillow from testing requirements.txt (elastic#20407)
  [Filebeat][ATP Module]Setting user agent field required by the API (elastic#20440)
  [Ingest Manager] Send datastreams fields (elastic#20402)
  Add event.ingested to all Filebeat modules (elastic#20386)
  [Elastic Agent] Fix agent control socket path to always be less than 107 characters (elastic#20426)
  Improve cgroup_regex docs with examples (elastic#20425)
  Makes `metrics` config option required in app_insights (elastic#20406)
  Ensure install scripts only install if needed (elastic#20349)
  Update container name for the azure filesets (elastic#19899)
  Group same timestamp metrics values in app_insights metricset (elastic#20403)
  add_process_metadata processor adds container id even if process metadata not accessible (elastic#19767)
  Support "cluster" scope in Metricbeat elasticsearch module (elastic#18547)
  [Filebeat][SophosXG Module] Renaming module and fileset (elastic#20396)
  Update Suricata dashboards (elastic#20394)
  [Elastic Agent] Improve version, restart, enroll CLI commands (elastic#20359)
  Prepare home directories for docker images in a different stage (elastic#20356)
  New multiline mode in Filebeat: while_pattern (elastic#19662)
  ...
melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020
…c#20403)

* mofidy doc

* work on data

* changelog

* mage fmt update
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…c#20403) (elastic#20419)

* mofidy doc

* work on data

* changelog

* mage fmt update

(cherry picked from commit fe78a57)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs_backport PR is waiting to be backported to other branches. Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team [zube]: In Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants