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

chore(agw): Update metrics naming on exporting #11668

Merged
merged 1 commit into from Mar 29, 2022

Conversation

ardzoht
Copy link
Contributor

@ardzoht ardzoht commented Feb 17, 2022

Summary

  • Currently, metrics collector on magmad is using a deprecated proto file (metricsd.proto) to set the name for each Metric family (collection of metrics on a service)
  • This change updates the filling of the metric name to use the name from the prometheus metric name rather from the protobuf description name.
  • Updates s1ap tests to remove metricsd proto enum values

Test Plan

  • make integ_test
  • running local environment and making sure metrics are still collected and exported successfully to the cloud:
Feb 08 22:45:33 magma-dev-focal magmad[51255]: DEBUG:root:Collected 9 from state...
Feb 08 22:45:33 magma-dev-focal magmad[51255]: DEBUG:root:Collected 18 from pipelined...
Feb 08 22:45:33 magma-dev-focal magmad[51255]: DEBUG:root:Metrics upload success for service pipelined (chunk 0)
Feb 08 22:45:33 magma-dev-focal magmad[51255]: DEBUG:root:Metrics upload success for service magmad (chunk 0)
Feb 08 22:45:33 magma-dev-focal magmad[51255]: DEBUG:root:Metrics upload success for service sessiond (chunk 0)
Feb 08 22:45:33 magma-dev-focal magmad[51255]: DEBUG:root:Metrics upload success for service state (chunk 0)
Feb 08 22:45:33 magma-dev-focal magmad[51255]: DEBUG:root:Metrics upload success for service enodebd (chunk 0)
  • service303_cli.py metrics pipelined before:
name: "50"
type: GAUGE
metric {
  label {
    name: "10"
    value: "3.8.5"
  }
  label {
    name: "11"
    value: "CPython"
  }
  label {
    name: "9"
    value: "3"
  }
  label {
    name: "8"
    value: "8"
  }
  label {
    name: "7"
    value: "5"
  }
  gauge {
    value: 1.0
  }
  timestamp_ms: 1644366222080
}

name: "309"
type: COUNTER

name: "550"
type: COUNTER
metric {
  counter {
    value: 2.0
  }
  timestamp_ms: 1644366222080
}

name: "350"
type: COUNTER

name: "351"
type: COUNTER
metric {
  counter {
    value: 0.0
  }
  timestamp_ms: 1644366222080
}

name: "352"
type: COUNTER

name: "353"
type: COUNTER
metric {
  counter {
    value: 0.0
  }
  timestamp_ms: 1644366222080
}
  • service303_cli.py metrics pipelined after:
name: "python_info"
type: GAUGE
metric {
  label {
    name: "10"
    value: "3.8.5"
  }
  label {
    name: "11"
    value: "CPython"
  }
  label {
    name: "9"
    value: "3"
  }
  label {
    name: "8"
    value: "8"
  }
  label {
    name: "7"
    value: "5"
  }
  gauge {
    value: 1.0
  }
  timestamp_ms: 1644374679693
}

name: "streamer_responses"
type: COUNTER

name: "service_errors"
type: COUNTER
metric {
  counter {
    value: 2.0
  }
  timestamp_ms: 1644374679693
}

name: "dp_send_msg_error"
type: COUNTER

name: "arp_default_gw_mac_error"
type: COUNTER
metric {
  counter {
    value: 0.0
  }
  timestamp_ms: 1644374679693
}

name: "openflow_error_msg"
type: COUNTER

name: "unknown_pkt_direction"
type: COUNTER
metric {
  counter {
    value: 0.0
  }
  timestamp_ms: 1644374679693
}

name: "enforcement_rule_install_fail"
type: COUNTER

name: "enforcement_stats_rule_install_fail"
type: COUNTER

Additional Information

  • This change is backwards-breaking

@ardzoht ardzoht requested review from a team, nstng, ssanadhya and themarwhal February 17, 2022 19:04
@pull-request-size pull-request-size bot added the size/XL Denotes a Pull Request that changes 500-999 lines. label Feb 17, 2022
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added component: agw Access gateway-related issue component: orc8r Orchestrator-related issue labels Feb 17, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2022

feg-workflow

    2 files  202 suites   32s ⏱️
362 tests 362 ✔️ 0 💤 0
376 runs  376 ✔️ 0 💤 0

Results for commit 4e54f54.

♻️ This comment has been updated with latest results.

@@ -29,12 +29,6 @@ pytest_test(
deps = ["//orc8r/gateway/python/magma/magmad:magmad_lib"],
)

pytest_test(
name = "metrics_tests",
srcs = ["metrics_tests.py"],
Copy link
Member

Choose a reason for hiding this comment

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

is this a deleted test?

Copy link
Member

Choose a reason for hiding this comment

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

oh i see it below :p

@@ -0,0 +1,220 @@
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason for file name change?

Copy link
Contributor

Choose a reason for hiding this comment

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

what is the difference from test_guti_attach_with_zero_mtmsi.py

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the connection of this test to the metrics naming.

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 think this file got included as part of a rebase with other commit I had on my local history, I will remove it, this is not supposed to be added

@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2022

cloud-workflow

    7 files  351 suites   2m 45s ⏱️
960 tests 960 ✔️ 0 💤 0

Results for commit 4e54f54.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2022

agw-workflow

     76 files     121 suites   5m 55s ⏱️
1 101 tests 1 092 ✔️ 9 💤 0
1 102 runs  1 093 ✔️ 9 💤 0

Results for commit 4e54f54.

♻️ This comment has been updated with latest results.

@pull-request-size pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/XL Denotes a Pull Request that changes 500-999 lines. labels Feb 17, 2022
Copy link
Contributor

@ulaskozat ulaskozat left a comment

Choose a reason for hiding this comment

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

LGTM

@ardzoht ardzoht force-pushed the update_metrics_naming branch 5 times, most recently from 14454b2 to 303a22c Compare March 2, 2022 16:17
Signed-off-by: Alex Rodriguez <alexrod@fb.com>
@ardzoht ardzoht merged commit c0f4b05 into magma:master Mar 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2022

Unit Test Results

157 files  157 suites   2h 6m 0s ⏱️
307 tests 304 ✔️ 3 💤 0

Results for commit c0f4b05.

♻️ This comment has been updated with latest results.

ardzoht added a commit that referenced this pull request Mar 30, 2022
Signed-off-by: Alex Rodriguez <alexrod@fb.com>
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue component: orc8r Orchestrator-related issue size/L Denotes a Pull Request that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants