[Elastic Agent]-PR- Introduce log message for not supported annotations for Hints based autodiscover #4360

gizas · 2024-03-05T15:57:08Z

~~Note: Dont merge until elastic/elastic-agent-autodiscover#81 is merged.~~
~~We need to udpate go.mod of the elastic-agent with v0.6.9~~ PR is merged

Enhancement

What does this PR do?

Relates to:

[Meta] Improving Hints Autodiscovery with Elastic Agent #3063
Introduce log message for not supported annotations for Hints based autodiscover of Elastic Agent #3064
Introduce checks for not supported annotations for Hints based autodiscover of Elastic Agent elastic-agent-autodiscover#81

Why is it important?

It adds a helpful feature for those that use Hints Autodiscovery. It can be helpful to identify configuration issues

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 (Some tests updated here)
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

Follow same instructions as #3107

Checkout this branch
Update hints.go file of your /go/pkg/mod/github.com/elastic/[email protected]/utils as per hints.go
Build elastic agent image

EXTERNAL=true DEV=true SNAPSHOT=true PLATFORMS=linux/arm64 PACKAGES=docker mage package
cd build/package/elastic-agent/elastic-agent-linux-arm64.docker/docker-build
docker build -t custom-agent-image .\n
kind load docker-image custom-agent-image:latest\n

Install elastic-agent-standalone with hints enabled. Sample here
elastic-agent-standalone.txt
Configure some "wrong" co.elastic.hints annotation inside a pod.

annotations:        
        co.elastic.hints/enabled: 'true'
        co.elastic.hints/package: "container_logs"
        co.elastic.hints/hostssssss: '128.30.40.50'

Warning messages should be available

Related issues

Relates Introduce log message for not supported annotations for Hints based autodiscover of Elastic Agent #3064

Logs

Deploy following pod with wrong annotations hosts and steam:

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/enabled: 'true'
        co.elastic.hints/hosts: '128.30.40.50'
        co.elastic.hints/steam: 'stdout'
    spec:
      containers:
      - image: nginx
        name: nginx

{"log.level":"warn","@timestamp":"2024-03-07T13:15:22.599Z","log.logger":"composable.providers.kubernetes","log.origin":{"file.name":"kubernetes/pod.go","file.line":219},"message":"provided hint: co.elastic/hints/hosts is not in the supported list for pod nginx-68d79864cb-rgjbb in namespace default","log":{"source":"elastic-agent"},"ecs.version":"1.6.0"}
{"log.level":"warn","@timestamp":"2024-03-07T13:15:22.599Z","log.logger":"composable.providers.kubernetes","log.origin":{"file.name":"kubernetes/pod.go","file.line":219},"message":"provided hint: co.elastic/hints/steam is not in the supported list for pod nginx-68d79864cb-rgjbb in namespace default","log":{"source":"elastic-agent"},"ecs.version":"1.6.0"}

Check that only annotations that start with co.elastic are the ones that are checked for correctness.

Screenshots

…in co.elastic.hints annotations

mergify · 2024-03-05T15:57:58Z

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 · 2024-03-05T15:59:19Z

I might need to add integration tests -> Added https://github.com/elastic/elastic-agent-autodiscover/pull/81/files#diff-2f7d9a00d75cb68d90813219822383fdda49c60b39cc05f7d2713f3d9e18a84a
~~To update changelog when is done~~
The for loops is also present in elastic-agent-autodiscover library. Wondering if I can put there the block of checks to minmimise the for loops that happen. Now we add iterate again to take annotations of pods I moved the implementation to elastic-agent-autodiscovery function for performance

gizas · 2024-03-06T14:01:42Z

internal/pkg/composable/providers/kubernetes/pod.go

+				//We check whether the provided annotation follows the supported format and vocabulary. The check happens for annotations that start with co.elastic.hints
+				hints, err := utils.GenerateHints(annotations, "", p.config.Prefix, allSupportedHints)
+				if err != nil {
+					p.logger.Warnf("%v for pod %v in namespace", err, pod.Name, pod.ObjectMeta.Namespace)


At the moment I set it as warning.
This kind of errors I think must be visible with default installation

But almost every 2sec we have a warning for one pod per wrong annotation. This can add noise
What do you think?

I think it should be added as a warning like you did, but having it every 2sec is quite annoying...

They will have it every two seconds until they fix it. If the only reason to get this warning is a wrong hint annotation, this will force them to fix it.
I would leave it as a warning.

elasticmachine · 2024-03-06T14:09:53Z

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

pchila · 2024-04-30T09:18:33Z

@pchila thank you for the inputs here.

I merged with main but still elastic-agent build fails. I see >> (linux-arm64-ubuntu-2204-default) Test output (non-sudo) (stdout): === FAIL: testing/integration TestOtelFileProcessing (188.30s) is the one that always fails

This is tracked by #4617 as it shows up on main as well.

Moreover for the Sonarcube coverage, my additions are made in private emitRunning function which is called on Add event. This fucntion was not covered even before introduction of SonarCube

I added a test here but obviously is not matched with specific function. Can we merge this despite the sonarcube failure?

Without having a green sonarqube check, someone with admin privileges should force the merge
I didn't have the time to fully review your merge yet but I would really prefer if we could cover new code/functionality with unit tests rather than force-merge without automated unit tests

gizas · 2024-05-01T06:54:46Z

/test

gizas · 2024-05-01T08:33:56Z

@pchila , @blakerouse I keep trying merging and with main. I hope that soon the build will work :)

I have also reformatted the code and introduced a function to call in order to create a test for coverage
Locally I see now that my code is covered:

But now I have another problem that SonarCube seems stack and never reports anything back.
Any ideas ?

gizas · 2024-05-01T08:34:17Z

/test

gizas · 2024-05-02T10:00:02Z

Nice!!!! it seems all tests are successful !!!!
@blakerouse , @pchila can I have final review and merge if no objection? (I have enabled auto-merge)

cmacknz · 2024-05-03T15:11:51Z

@blakerouse please re-review, looks like go.mod has been fixed.

cmacknz · 2024-05-03T20:11:40Z

buildkite test it

mergify · 2024-05-07T21:52:08Z

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 logshints upstream/logshints
git merge upstream/main
git push upstream logshints

…shints

gizas · 2024-05-08T08:38:36Z

@blakerouse please any objection before merging?

mergify · 2024-05-08T18:24:07Z

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 logshints upstream/logshints
git merge upstream/main
git push upstream logshints

gizas · 2024-05-09T06:43:26Z

/test

gizas · 2024-05-09T14:10:24Z

@blakerouse another ping because the merging is blocked here

blakerouse

Looks good.

elastic-sonarqube · 2024-05-09T14:48:00Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
83.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

gizas added 2 commits March 5, 2024 17:00

first commit with checks for hints vocabulary

c60baaa

updating code to postition in correct place and to be triggered only …

2148751

…in co.elastic.hints annotations

mergify bot assigned gizas Mar 5, 2024

mergify bot added the backport-skip label Mar 5, 2024

gizas added 4 commits March 5, 2024 18:21

fixing to retrieve only keys keys

664ebea

updating comment

f44c16f

adding changelog fragment

56d5015

fixing linting issues

ffa6e15

gizas mentioned this pull request Mar 6, 2024

Introduce checks for not supported annotations for Hints based autodiscover of Elastic Agent elastic/elastic-agent-autodiscover#81

Merged

gizas added 2 commits March 6, 2024 14:22

moved checks in elastic-agent-autodicover

b0fd369

correcting namespace and adding comment

ea314f9

gizas marked this pull request as ready for review March 6, 2024 12:26

gizas requested review from a team as code owners March 6, 2024 12:26

gizas requested review from gsantoro, constanca-m, AndersonQ, pchila, MichaelKatsoulis and tetianakravchenko March 6, 2024 12:26

Merge branch 'main' into logshints

5da1316

gizas commented Mar 6, 2024

View reviewed changes

pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Mar 6, 2024

pierrehilbert requested a review from blakerouse March 6, 2024 14:09

gizas added 3 commits March 6, 2024 17:05

adding checks also to containers

6536f3a

udpating comment

2aea421

fixing forloop error with value

bae96be

gizas added 2 commits April 30, 2024 17:15

adding function to override sonarcube

88cb012

adding function to override sonarcube

fe3548f

Merge branch 'main' into logshints

2512f64

Merge branch 'main' into logshints

dfb1945

gizas enabled auto-merge (squash) May 2, 2024 09:59

ycombinator requested a review from blakerouse May 2, 2024 12:32

gizas added 2 commits May 2, 2024 15:41

Merge branch 'main' into logshints

0cb2591

Merge branch 'main' into logshints

e8318ae

cmacknz mentioned this pull request May 3, 2024

Bump github.com/elastic/elastic-agent-autodiscover from 0.6.8 to 0.6.14 #4616

Merged

Merge branch 'main' into logshints

c191f4b

gizas added 2 commits May 8, 2024 09:43

merging with main

d32fe34

Merge branch 'logshints' of github.com:elastic/elastic-agent into log…

fd7dcfd

…shints

gizas added 2 commits May 9, 2024 09:46

merging with main

e6c8118

Merge branch 'main' into logshints

49af807

blakerouse approved these changes May 9, 2024

View reviewed changes

gizas merged commit 62958d0 into main May 9, 2024
9 checks passed

gizas deleted the logshints branch May 9, 2024 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Elastic Agent]-PR- Introduce log message for not supported annotations for Hints based autodiscover #4360

[Elastic Agent]-PR- Introduce log message for not supported annotations for Hints based autodiscover #4360

gizas commented Mar 5, 2024 •

edited

Loading

mergify bot commented Mar 5, 2024

gizas commented Mar 5, 2024 •

edited

Loading

gizas Mar 6, 2024 •

edited

Loading

constanca-m Mar 6, 2024

MichaelKatsoulis Mar 6, 2024

elasticmachine commented Mar 6, 2024

pchila commented Apr 30, 2024

gizas commented May 1, 2024

gizas commented May 1, 2024 •

edited

Loading

gizas commented May 1, 2024

gizas commented May 2, 2024

cmacknz commented May 3, 2024

cmacknz commented May 3, 2024

mergify bot commented May 7, 2024

gizas commented May 8, 2024

mergify bot commented May 8, 2024

gizas commented May 9, 2024

gizas commented May 9, 2024

blakerouse left a comment

elastic-sonarqube bot commented May 9, 2024

[Elastic Agent]-PR- Introduce log message for not supported annotations for Hints based autodiscover #4360

[Elastic Agent]-PR- Introduce log message for not supported annotations for Hints based autodiscover #4360

Conversation

gizas commented Mar 5, 2024 • edited Loading

What does this PR do?

Why is it important?

How to test this PR locally

Related issues

Logs

Screenshots

mergify bot commented Mar 5, 2024

gizas commented Mar 5, 2024 • edited Loading

gizas Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

constanca-m Mar 6, 2024

Choose a reason for hiding this comment

MichaelKatsoulis Mar 6, 2024

Choose a reason for hiding this comment

elasticmachine commented Mar 6, 2024

pchila commented Apr 30, 2024

gizas commented May 1, 2024

gizas commented May 1, 2024 • edited Loading

gizas commented May 1, 2024

gizas commented May 2, 2024

cmacknz commented May 3, 2024

cmacknz commented May 3, 2024

mergify bot commented May 7, 2024

gizas commented May 8, 2024

mergify bot commented May 8, 2024

gizas commented May 9, 2024

gizas commented May 9, 2024

blakerouse left a comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented May 9, 2024

Quality Gate passed

gizas commented Mar 5, 2024 •

edited

Loading

gizas commented Mar 5, 2024 •

edited

Loading

gizas Mar 6, 2024 •

edited

Loading

gizas commented May 1, 2024 •

edited

Loading