Skip to content

chore: extend the helm template for spanDropFilter#298

Merged
kotharironak merged 10 commits intomainfrom
add-helm-config-for-span-drop-filter
Jan 7, 2022
Merged

chore: extend the helm template for spanDropFilter#298
kotharironak merged 10 commits intomainfrom
add-helm-config-for-span-drop-filter

Conversation

@kotharironak
Copy link
Copy Markdown
Contributor

This PR makes spanDropFilter configurable via helm tempale.

Tested locally by running helm template ./helm

Input into values.yaml file:

processor:
    spanDropFilters: |-
      [
        [
          {
            "tagKey": "http.method",
            "operator": "EQ",
            "tagValue": "GET"
          },
          {
            "tagKey": "http.url",
            "operator": "CONTAINS",
            "tagValue": "health"
          }
        ],
        [
          {
            "tagKey": "grpc.url",
            "operator": "NEQ",
            "tagValue": "Sent.TestServiceGetEchos"
          }
        ]
      ]

Output:

# Source: span-normalizer/templates/span-normalizer-config.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: span-normalizer-config
  labels:
    release: RELEASE-NAME
data:
  application.conf: |-
    kafka.streams.config {
      application.id = jaeger-spans-to-raw-spans-job
      <removed>
    }
    processor {
      spanDropFilters = [
      [
        {
          "tagKey": "http.method",
          "operator": "EQ",
          "tagValue": "GET"
        },
        {
          "tagKey": "http.url",
          "operator": "CONTAINS",
          "tagValue": "health"
        }
      ],
      [
        {
          "tagKey": "grpc.url",
          "operator": "NEQ",
          "tagValue": "Sent.TestServiceGetEchos"
        }
      ]
    ]
    }
---
# Source: span-normalizer/templates/deployment.yaml

Test example: https://github.com/hypertrace/hypertrace-ingester/blob/main/span-normalizer/span-normalizer/src/test/resources/configs/span-normalizer/application.conf#L30

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 6, 2022

Codecov Report

Merging #298 (0e24543) into main (7ee3a8e) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #298      +/-   ##
============================================
+ Coverage     79.58%   79.60%   +0.01%     
- Complexity     1286     1287       +1     
============================================
  Files           116      116              
  Lines          5128     5128              
  Branches        465      465              
============================================
+ Hits           4081     4082       +1     
  Misses          836      836              
+ Partials        211      210       -1     
Flag Coverage Δ
unit 79.60% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...race/core/rawspansgrouper/TraceEmitPunctuator.java 75.43% <0.00%> (+0.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ee3a8e...0e24543. Read the comment docs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

{{- end }}

{{- if hasKey .Values.spanNormalizerConfig.processor "spanDropFilters" }}
spanDropFilters = {{ .Values.spanNormalizerConfig.processor.spanDropFilters | indent 4 | trim }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

toJson not required here?

Copy link
Copy Markdown
Contributor Author

@kotharironak kotharironak Jan 6, 2022

Choose a reason for hiding this comment

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

I am not loading as JSON, directly reading as a config object. See this example - https://github.com/hypertrace/hypertrace-ingester/blob/main/span-normalizer/span-normalizer/src/test/resources/configs/span-normalizer/application.conf#L30

I observed here that other configs were converted toJson string, will check out that were they read as a string.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But the value that you are providing in the config is a json, this indent and trim function applies to string

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As shown in the description, I was using the marker |- in YAML that is multi-string notation. So, the entire thing was treated as a big string, and If I apply the toJson function to it, it jsonify the string. As an example below.

Input from the above PR description:

processor:
    spanDropFilters: |-
      [
        [
          {
            "tagKey": "http.method",
            "operator": "EQ",
            "tagValue": "GET"
          },
          {
            "tagKey": "http.url",
            "operator": "CONTAINS",
            "tagValue": "health"
          }
        ],
        [
          {
            "tagKey": "grpc.url",
            "operator": "NEQ",
            "tagValue": "Sent.TestServiceGetEchos"
          }
        ]
      ]

Output with toJson on multi-line string using YAML marker |-:

processor {
      spanDropFilters = "[\n  [\n    {\n      \"tagKey\": \"http.method\",\n      \"operator\": \"EQ\",\n      \"tagValue\": \"GET\"\n    },\n    {\n      \"tagKey\": \"http.url\",\n      \"operator\": \"CONTAINS\",\n      \"tagValue\": \"health\"\n    }\n  ],\n  [\n    {\n      \"tagKey\": \"grpc.url\",\n      \"operator\": \"NEQ\",\n      \"tagValue\": \"Sent.TestServiceGetEchos\"\n    }\n  ]\n]"
    }

And, so there was no need for toJson function. However, I modified the code to not use |- multi-string marker, and treat the value as a YAML for spanDropFilters. As you guys pointed out, this will be better to catch any config error by converting it to toJson, so going it with.

So, the input will be:

processor:
    spanDropFilters:
      [
        [
          {
            "tagKey": "http.method",
            "operator": "EQ",
            "tagValue": "GET"
          },
          {
            "tagKey": "http.url",
            "operator": "CONTAINS",
            "tagValue": "health"
          }
        ],
        [
          {
            "tagKey": "grpc.url",
            "operator": "NEQ",
            "tagValue": "Sent.TestServiceGetEchos"
          }
        ]
      ]

Output for above using: {{ .Values.spanNormalizerConfig.processor.spanDropFilters | toJson }}

processor {
      spanDropFilters = [[{"operator":"EQ","tagKey":"http.method","tagValue":"GET"},{"operator":"CONTAINS","tagKey":"http.url","tagValue":"health"}],[{"operator":"NEQ","tagKey":"grpc.url","tagValue":"Sent.TestServiceGetEchos"}]]
    }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh ok |- this was present

@github-actions

This comment has been minimized.

Comment thread .snyk
- '*':
reason: no available replacement
expires: 2021-12-31T00:00:00.000Z
expires: 2022-01-31T00:00:00.000Z
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what does this mean ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As currently there is no fixed version available for this (https://security.snyk.io/vuln/SNYK-JAVA-IONETTY-1042268), I am extending the expiry date for one month, till then it will not be reported as snyk failures.

Do you see anything wrong or missing something? It's YYYY-MM-DD, right?

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

ravisingal
ravisingal previously approved these changes Jan 7, 2022
@kotharironak
Copy link
Copy Markdown
Contributor Author

Added constraints for view-gen framework in this repo, we will update the view-gen separately (hypertrace/view-generator-framework#53).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

findingrish
findingrish previously approved these changes Jan 7, 2022
"[Medium Severity][https://snyk.io/vuln/SNYK-JAVA-COMFASTERXMLJACKSONCORE-2326698] " +
"in com.fasterxml.jackson.core:jackson-databind@2.12.2")
}
implementation("org.apache.logging.log4j:log4j-slf4j-impl:2.17.1") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should update log4j version in view-generator-framework repository.

implementation("com.fasterxml.jackson.core:jackson-databind:2.13.1")

constraints {
implementation("org.apache.logging.log4j:log4j-slf4j-impl:2.17.1") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should update log4j version in view-generator-framework repository instead of adding constraint.

@kotharironak
Copy link
Copy Markdown
Contributor Author

@rish691 @ravisingal Took care of log4j vuln vai fixing view-gen framework - hypertrace/view-generator-framework#53. Pl. re-approve.

@github-actions

This comment has been minimized.

@kotharironak kotharironak merged commit f63f098 into main Jan 7, 2022
@kotharironak kotharironak deleted the add-helm-config-for-span-drop-filter branch January 7, 2022 14:19
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 7, 2022

Unit Test Results

  75 files  ±0    75 suites  ±0   59s ⏱️ -11s
394 tests ±0  394 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f63f098. ± Comparison against base commit 7ee3a8e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants