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

Support Hints per container input #3416

Merged
merged 14 commits into from
Oct 6, 2023
Merged

Support Hints per container input #3416

merged 14 commits into from
Oct 6, 2023

Conversation

gizas
Copy link
Contributor

@gizas gizas commented Sep 14, 2023

Enhancement

What does this PR do?

This PR is introrduces the functionality described in #3161!
In more details users provide the container name in the hint and this functionality is applied only to a specific container of the pod. When a pod has multiple containers, the settings are shared unless you put the container name in the hint.

For eg. co.elastic.hints.nginx/stream: stderr is an annotation (aka. hints) that will be applied to a container wtih name nginx

Why is it important?

It introduces a missing functionality that exists in Hints autodiscovry for beats as described in the Multiple Containers Section

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/fragments using the changelog tool
  • I have added an integration test or an E2E test

How to test this PR locally

  1. Follow the same steps as desceibed in "How to test this PR locally" of Add support for processors in hints based k8s autodiscover #3107
  2. Apply the nginx.yaml manifest that creates a pod with 2 containers
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: nginx
  name: nginx
  namespace: default
spec:
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
      annotations:
        co.elastic.hints/package: "container_logs"
        co.elastic.hints/processors.decode_json_fields.fields: "message"
        co.elastic.hints/processors.decode_json_fields.add_error_key: 'true'
        co.elastic.hints/processors.decode_json_fields.overwrite_keys: 'true'
        co.elastic.hints/processors.decode_json_fields.target: "team"
        co.elastic.hints.nginx/stream: stderr
        co.elastic.hints.nginx/processors.add_fields.fields.name: "myproject"
    spec:
      containers:
      - image: nginx
        name: nginx
      - image: training/webapp
        name: webapp
      
  1. Exec into Elastic Agent
kubectl exec -ti -n kube-system elastic-agent-t95bj -- bash

/share/elastic-agent# elastic-agent inspect -v --variables --variables-wait 2s

Related issues

Logs

Output file generated with inspect command
agent.txt

id: hints-filestream-container-logs-a3263d2b196abc0695f1ba1767cce0d0d40cef186cce132d3b39dcfb84ec74a0-kubernetes-9b52738c-3366-4f86-8fb3-bfefddd65ef0.nginx
[output .truncated..]
- decode_json_fields:    < ----- added from co.elastic.hints/processors.decode_json_fields.* 
      add_error_key: "true"
      fields: message
      overwrite_keys: "true"
      target: team

#<----- Added only to this container from co.elastic.hints.nginx/processors.add_fields.fields.name: "myproject"
  - add_fields:   
      fields:
        name: myproject
  streams:
  - data_stream:
      dataset: kubernetes.container_logs
      type: logs
    parsers:
    - container:
        format: auto
        stream: stderr. <-- From co.elastic.hints.nginx/stream: stderr

....

id: hints-filestream-container-logs-c28563d37f000e2b7a31f014ba403a0bc16a94dafc63169bc0f98143fc96832c-kubernetes-9b52738c-3366-4f86-8fb3-bfefddd65ef0.webapp
- decode_json_fields: < ----- added from co.elastic.hints/processors.decode_json_fields.* of 
      add_error_key: "true"
      fields: message
      overwrite_keys: "true"
      target: team
  streams:
  - data_stream:
      dataset: kubernetes.container_logs
      type: logs
    parsers:
    - container:
        format: auto
        stream: all. <---- This did not chane to stderr
##<----- Did not affected from   co.elastic.hints.nginx/processors.add_fields.fields.name: "myproject"

@gizas gizas requested a review from a team as a code owner September 14, 2023 11:45
@mergify mergify bot assigned gizas Sep 14, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 14, 2023

This pull request does not have a backport label. Could you fix it @gizas? 🙏
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 8./d branch. /d is the digit

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

@gizas
Copy link
Contributor Author

gizas commented Sep 14, 2023

  • Documentation updates will be handled in another PR (TBD)
  • I will need to do some manual tests with metrics
  • Probably to update the integration tests

@gizas gizas requested a review from a team as a code owner September 14, 2023 11:54
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 14, 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-10-06T14:39:21.103+0000

  • Duration: 30 min 36 sec

Test stats 🧪

Test Results
Failed 0
Passed 6489
Skipped 59
Total 6548

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

  • /package : Generate the packages.

  • run integration tests : Run the Elastic Agent Integration tests.

  • run end-to-end tests : Generate the packages and run the E2E Tests.

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

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

I think this one would need some unit tests so as to ensure the added functionality.

internal/pkg/composable/providers/kubernetes/hints.go Outdated Show resolved Hide resolved
internal/pkg/composable/providers/kubernetes/hints.go Outdated Show resolved Hide resolved
internal/pkg/composable/providers/kubernetes/hints.go Outdated Show resolved Hide resolved
internal/pkg/composable/providers/kubernetes/hints.go Outdated Show resolved Hide resolved
internal/pkg/composable/providers/kubernetes/hints.go Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 20, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.78% (81/82) 👍
Files 67.003% (199/297) 👍
Classes 65.642% (363/553) 👍
Methods 52.974% (1149/2169) 👍 0.043
Lines 38.563% (13128/34043) 👍 0.094
Conditionals 100.0% (0/0) 💚

@constanca-m
Copy link
Contributor

constanca-m commented Sep 28, 2023

Great work @gizas !

I tested it with 1 pod running 2 nginx containers: one with port 80 and another with port 81.

I added the annotations to have the correct host for each container:

co.elastic.hints/package: nginx
co.elastic.hints/data_streams: stubstatus
co.elastic.hints.nginx-2/host: '${kubernetes.pod.ip}:81'
co.elastic.hints.nginx-1/host: '${kubernetes.pod.ip}:80'

And there it was (the barchart is for the unique count of container.name):
image

My two containers were there. I also tested with just one host, and it worked.

@bartoszcisek
Copy link

bartoszcisek commented Sep 28, 2023

Great stuff! Can't wait for this one to be released ;-)

// mappings.container.name = nginx defines the container we want to emmit the new configuration. Annotations for other containers like co.elastic.hints.webapp should be excluded
func TestGenerateHintsMappingWithProcessorsForContainer(t *testing.T) {
logger := getLogger()
// pod := &kubernetes.Pod{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@constanca-m I commented out the pod section! Just kept it for reference with above tests.

Test is successful

Let me know if is ok or you think we can remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok with me

@gizas
Copy link
Contributor Author

gizas commented Oct 5, 2023

/test

3 similar comments
@gizas
Copy link
Contributor Author

gizas commented Oct 5, 2023

/test

@gizas
Copy link
Contributor Author

gizas commented Oct 6, 2023

/test

@gizas
Copy link
Contributor Author

gizas commented Oct 6, 2023

/test

Copy link

@devamanv devamanv left a comment

Choose a reason for hiding this comment

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

LGTM, just left a couple of small comments.

gizas and others added 2 commits October 6, 2023 17:38
Co-authored-by: Aman Verma <38116245+devamanv@users.noreply.github.com>
Co-authored-by: Aman Verma <38116245+devamanv@users.noreply.github.com>
@elastic-sonarqube
Copy link

Copy link
Contributor

@pchila pchila left a comment

Choose a reason for hiding this comment

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

LGTM

@gizas gizas merged commit 7a667da into main Oct 6, 2023
24 checks passed
@gizas gizas deleted the hintspercontainer branch October 6, 2023 17:36
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

8 participants