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

Add k8s metadata in state_cronjob metricset #29572

Merged
merged 6 commits into from
Jan 11, 2022

Conversation

ChrsMark
Copy link
Member

What does this PR do?

This PR adds metadata enrichment for state_cronjob metricset.

Why is it important?

So as to be aligned with other metricsets.

Related issues

Logs

Sample event

{
  "@timestamp": "2021-12-22T08:12:32.510Z",
  "@metadata": {
    "beat": "metricbeat",
    "type": "_doc",
    "version": "8.1.0"
  },
  "orchestrator": {
    "cluster": {
      "url": "kind-control-plane:6443",
      "name": "kind"
    }
  },
  "ecs": {
    "version": "8.0.0"
  },
  "host": {
    "name": "kind-control-plane"
  },
  "agent": {
    "name": "kind-control-plane",
    "type": "metricbeat",
    "version": "8.1.0",
    "ephemeral_id": "5b73917e-a756-4a78-9a26-db5fd108c1c5",
    "id": "8d46ce8b-bcd6-41f4-baa4-c2af42952625"
  },
  "service": {
    "address": "http://kube-state-metrics:8080/metrics",
    "type": "kubernetes"
  },
  "event": {
    "dataset": "kubernetes.cronjob",
    "module": "kubernetes",
    "duration": 315608278
  },
  "kubernetes": {
    "cronjob": {
      "is_suspended": false,
      "next_schedule": {
        "sec": 1.64016078e+09
      },
      "last_schedule": {
        "sec": 1.64016072e+09
      },
      "created": {
        "sec": 1.640159561e+09
      },
      "active": {
        "count": 0
      },
      "name": "hello"
    },
    "labels": {
      "foo": "bar"
    },
    "namespace": "default"
  },
  "metricset": {
    "period": 10000,
    "name": "state_cronjob"
  }
}

Signed-off-by: chrismark <chrismarkou92@gmail.com>
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Dec 22, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 22, 2021

This pull request does not have a backport label. Could you fix it @ChrsMark? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Dec 22, 2021
@ChrsMark ChrsMark self-assigned this Dec 22, 2021
@ChrsMark ChrsMark added the Team:Integrations Label for the Integrations team label Dec 22, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 22, 2021
@ChrsMark ChrsMark added the backport-v8.1.0 Automated backport with mergify label Dec 22, 2021
@mergify mergify bot removed the backport-skip Skip notification from the automated backport with mergify label Dec 22, 2021
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@elasticmachine
Copy link
Collaborator

elasticmachine commented Dec 22, 2021

💚 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: 2022-01-10T14:35:59.148+0000

  • Duration: 131 min 0 sec

  • Commit: 9e7177c

Test stats 🧪

Test Results
Failed 0
Passed 47267
Skipped 4155
Total 51422

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

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

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Copy link
Contributor

@tetianakravchenko tetianakravchenko left a comment

Choose a reason for hiding this comment

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

LGTM! I've left few questions

@@ -88,6 +88,7 @@ func (p *pod) GenerateK8s(obj kubernetes.Resource, opts ...FieldOptions) common.
out := p.resource.GenerateK8s("pod", obj, opts...)

// check if Pod is handled by a ReplicaSet which is controlled by a Deployment
// TODO: same happens with CronJob vs Job. The hierarcy there is CronJob->Job->Pod
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a ticket for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -30,7 +30,7 @@ import (
)

func TestFetchMetricset(t *testing.T) {
config := test.GetKubeStateMetricsConfig(t, "state_cronjob")
config := test.GetKubeStateMetricsConfigWithMetaDisabled(t, "state_cronjob")
Copy link
Contributor

Choose a reason for hiding this comment

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

why for cronjob only add_metadata must be disabled? as I see for other k8s resources used default value - true

Copy link
Member Author

Choose a reason for hiding this comment

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

Cause in version before v1.21 the the current stable api is not available and hence we cannot support metadata in tests for v1.20

@@ -118,7 +118,9 @@ func (r *Resource) GenerateK8s(kind string, obj kubernetes.Resource, options ...
case "Deployment",
"ReplicaSet",
"StatefulSet",
"DaemonSet":
"DaemonSet",
"Job",
Copy link
Contributor

@tetianakravchenko tetianakravchenko Dec 23, 2021

Choose a reason for hiding this comment

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

with this change we also can close this PR: #28954 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tetianakravchenko my PR is a little bit tricky cause has some tricky parts around k8s api versions. I think we can move on with the community PR and I can rebase mine on top of it. wdyt?

@mergify
Copy link
Contributor

mergify bot commented Dec 26, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b k8s_cron_meta_enricher upstream/k8s_cron_meta_enricher
git merge upstream/master
git push upstream k8s_cron_meta_enricher

@ChrsMark
Copy link
Member Author

/test

@ChrsMark
Copy link
Member Author

@MichaelKatsoulis @tetianakravchenko I will need a final review on this. I changed a little bit the behaviour and the add_metadata setting is disabled by default in state_cronjob metricset until we only support stable versions of the corresponding API.

Copy link
Contributor

@tetianakravchenko tetianakravchenko left a comment

Choose a reason for hiding this comment

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

LGTM!

@ChrsMark ChrsMark merged commit 2f79c77 into elastic:master Jan 11, 2022
v1v added a commit to v1v/beats that referenced this pull request Jan 12, 2022
…b-for-macos

* upstream/master: (172 commits)
  [Elastic Agent] Fix issue with ensureServiceToken. (elastic#29800)
  [Winlogbeat] Add provider name to Security routing pipeline check (elastic#29781)
  Add summary to journeys which don't emit journey:end (early node subprocess exits) (elastic#29606)
  Prepare 8.0.0-rc1 changelog (elastic#29795) (elastic#29806)
  Change docker image from CentOS 7 to Ubuntu 20.04 (elastic#29681)
  libbeat/processors/add_process_metadata: implement a process cache eviction policy (elastic#29717)
  [Automation] Update elastic stack version to 8.1.0-7004acda for testing (elastic#29783)
  Missing changelog entry for elastic#29773 (elastic#29791)
  Add a readme for k8s autodiscover provider (elastic#28213)
  Remove overriding of index pattern on the Kubernetes overview dashboard (elastic#29676)
  jjbb: remove obsoleted branches (<7.16) (elastic#29707)
  Add k8s metadata in state_cronjob metricset (elastic#29572)
  ibmmq: Fix timestamp parsing (elastic#29773)
  Do not add date to index if `@meta.index` is set (elastic#29775)
  ci: uses aliases for the branches (elastic#29706)
  Filebeat tests: Restore `@timestamp` field validation (elastic#29772)
  Forward port 7.16.3 changelog to master (elastic#29777)
  auditd: Store program arguments in process.args array (elastic#29601)
  System/socket: Support kernel_clone() replacement for _do_fork() (elastic#29744)
  Do not mention removal if version is not specified in `cfgwarn` messages (elastic#29727)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.1.0 Automated backport with mergify Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

state_cronjob lacks enricher
3 participants