feat: report attributes as fields #874

pdelewski · 2022-12-19T10:56:11Z

There is opened bug https://jira.kumoroku.com/jira/browse/SUMO-207676, which is about reporting severity as a field. This PR does similar thing as LokiExporter, adding severity into set of attributes. It's preliminary solution, showing only possible way, not necessarily final solution and question is how do we want to tackle it.

swiatekm · 2022-12-19T11:41:51Z

This shouldn't happen in the exporter, as exporters shouldn't modify data. I think this should be a feature in the schema processor, if anything, and should probably be configurable.

aboguszewski-sumo · 2022-12-19T11:56:03Z

I agree that this should be in a processor. It also seems to be totally independent from what I'm doing right now in sumologicschema processor, so feel free to add it there as soon as you need it.

pdelewski · 2022-12-19T14:20:28Z

This shouldn't happen in the exporter, as exporters shouldn't modify data. I think this should be a feature in the schema processor, if anything, and should probably be configurable.

Thx. Agree, that it should be configurable

aboguszewski-sumo · 2022-12-22T10:40:56Z

A few nits. Please modify README.md and add a sample test to processor_test.go

pkg/processor/sumologicschemaprocessor/log_fields_conversion_processor.go

pkg/processor/sumologicschemaprocessor/processor.go

pkg/processor/sumologicschemaprocessor/log_fields_conversion_processor.go

pkg/processor/sumologicschemaprocessor/testdata/config.yaml

pkg/processor/sumologicschemaprocessor/log_fields_conversion_processor.go

andrzej-stencel · 2022-12-28T09:29:21Z

pkg/processor/sumologicschemaprocessor/README.md

+
+    # Defines whether `severity` attribute should be added to record attributes.
+    # default = true
+    add_severity_level_attribute: {true, false}


I propose the following configuration, or something similar:

add_severity_attribute: enabled: true attribute_name: loglevel

Changed configuration and extended to use other fields as well, eg. severity text span id and trace id. That was asked by our customer support, however I'm still waiting for confirmation if client needs that. Anyway, we can always leave it or remove.

shouldn't this be updated?

I agree with @astencel-sumo that the configuration added by this PR should be grouped in one major key, instead of producing 4 new config options

@sumo-drosiek I don't think @astencel-sumo suggested grouping, but maybe I'm wrong?

@astencel-sumo? 🤔

Hi @pdelewski any update on this? The customer is inquiring for updates and I don't see any recent movement. Thank you.

Hi @bnartiff , I will update PR tomorrow. It still requires aprovals. I was also waiting for confirmation regarding fields that customer expects (as originally there were mentioned only severity, recently I updated that to spanid and traceid

Thanks for the update, I am going to check w/ the customer to confirm the desired fields. Thank you for adding spanid and traceid as well.

Per customer, "I think severity, spanID, and traceID are a good start. I don’t have any other suggestions/needs at this point. " @pdelewski

sumo-drosiek · 2023-01-10T13:57:20Z

pkg/processor/sumologicschemaprocessor/README.md

+
+In order to report one of them as field, following configuration is needed:
+
+```json


Suggested change

```json

```yaml

pdelewski · 2023-01-24T13:53:13Z

@astencel-sumo @sumo-drosiek @aboguszewski-sumo Could you look at this PR?

aboguszewski-sumo

I have some problems with understanding how should this work in general. As I understand - there are some OTLP fields in logs that are not shown in Sumo, and to fix that, we transform them into regular attributes. Should every of these fields be translated, or only one, selected in the config? The implementation suggests that all of them are translated, but the config suggests that only one of them.

aboguszewski-sumo · 2023-01-24T14:03:04Z

pkg/processor/sumologicschemaprocessor/README.md

+
+### Severity Attribute
+
+Some fields that log entries consist are not displayed as fields in Sumo Logic out of the box.


Suggested change

Some fields that log entries consist are not displayed as fields in Sumo Logic out of the box.

Some fields that log entries consist of are not displayed as fields in Sumo Logic out of the box.

aboguszewski-sumo · 2023-01-24T14:05:38Z

pkg/processor/sumologicschemaprocessor/README.md

+
+    # Defines whether `severity` attribute should be added to record attributes.
+    add_severity_number_attribute:
+        enabled: true
+        attribute_name: "loglevel"
+


Keep it consistent with other examples above in this file, eg.:

add_severity_number_attribute: # if enabled, then XYZ # default = true enabled: {true, false} # The name that is something # default = "loglevel" attribute_name: "loglevel"

aboguszewski-sumo · 2023-01-24T14:11:55Z

pkg/processor/sumologicschemaprocessor/README.md

+In order to report one of them as field, following configuration is needed:
+
+```yaml
+    add_severity_number_attribute:
+        enabled: true
+        attribute_name: "loglevel"
+```


Can all of them be enabled or only one at one time? Maybe it's better to squash this into one attribute and disable it by selecting none, empty string or something like that?

My idea was to be able to enable one by one, however I have not seen requirements regarding this from customer side.

Ok, I've read the discussion above. In my opinion, the best thing would be to define something like this (the name should probably be different, I'm not sure how to call all these attributes):

add_log_metadata_attributes: metadata: ["severity_no", "severity_text", "span_id", "trace_id"]

ok, I can update implementation according to your suggestion, @astencel-sumo @sumo-drosiek What's your opinion on that? I would like to avoid yet another round of changes regarding configuration.

I'm not convinced that squashing all configurations into a single property is any better than the current solution. We can discuss in a meeting.

I'm not convinced that squashing all configurations into a single property is any better than the current solution. We can discuss in a meeting.

same as we skip attribute_name

aboguszewski-sumo · 2023-01-24T14:23:21Z

pkg/processor/sumologicschemaprocessor/processor_test.go

+			name:        "addSeverity",
+			addSeverity: true,
+			createLogs: func() plog.Logs {
+				logs := plog.NewLogs()
+				log := logs.ResourceLogs().AppendEmpty().ScopeLogs().AppendEmpty().LogRecords().AppendEmpty()
+				log.SetSeverityNumber(plog.SeverityNumberInfo)
+				log.SetSeverityText("severity")
+				var spanIDBytes [8]byte = [8]byte{1, 1, 1, 1, 1, 1, 1, 1}
+				log.SetSpanID(spanIDBytes)
+				var traceIDBytes [16]byte = [16]byte{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}
+				log.SetTraceID(traceIDBytes)
+				return logs
+			},
+			test: func(outputLogs plog.Logs) {
+				attribute1, found := outputLogs.ResourceLogs().At(0).ScopeLogs().At(0).LogRecords().At(0).Attributes().Get("loglevel")
+				assert.True(t, found)
+				assert.Equal(t, "INFO", attribute1.Str())
+				attribute2, found := outputLogs.ResourceLogs().At(0).ScopeLogs().At(0).LogRecords().At(0).Attributes().Get("severitytext")
+				assert.True(t, found)
+				assert.Equal(t, "severity", attribute2.Str())
+				attribute3, found := outputLogs.ResourceLogs().At(0).ScopeLogs().At(0).LogRecords().At(0).Attributes().Get("spanid")
+				assert.True(t, found)
+				assert.Equal(t, "0101010101010101", attribute3.Str())
+				attribute4, found := outputLogs.ResourceLogs().At(0).ScopeLogs().At(0).LogRecords().At(0).Attributes().Get("traceid")
+				assert.True(t, found)
+				assert.Equal(t, "01010101010101010101010101010101", attribute4.Str())
+
+			},
+		},


I'm not sure how this works.
In the config (lines 1356+), you set only SeverityNumberAttributeName to be transformed, but here everything is transformed.

Will check that

aboguszewski-sumo · 2023-01-24T14:28:17Z

pkg/processor/sumologicschemaprocessor/README.md

+
+`add_severity_number_attribute`
+`add_severity_text_attribute`
+`add_space_id_attribute`


Suggested change

`add_space_id_attribute`

`add_span_id_attribute`

andrzej-stencel · 2023-01-25T08:37:05Z

pkg/processor/sumologicschemaprocessor/processor_test.go

@@ -1298,3 +1353,15 @@ func newAggregateAttributesConfig(aggregations []aggregationPair) *Config {
 	config.AggregateAttributes = aggregations
 	return config
 }
+
+func newLogFieldsConversionConfig(logFieldsConversion bool) *Config {


The parameter logFieldsConversion does not seem to be used? Which probably indicates that the test coverage is not good enough 😉

@sumo-drosiek @astencel-sumo @aboguszewski-sumo I updated PR according to our today discussion as well as fixed some bugs. Current configuration looks as suggested by @sumo-drosiek, however I have not changed main key field_attributes yet. (I'm looking for better alternative and inspiration )

field_attributes: trace_id: enabled: true name: traceid span_id: enabled: true name: spanid

If you have suggestion regarding replacing fieldd_attributes with better name, just let me know

aboguszewski-sumo · 2023-02-01T11:36:02Z

pkg/processor/sumologicschemaprocessor/README.md

+        enabled: true
+        name: "loglevel"
+
+    - ...


this line (79) seems redundant

aboguszewski-sumo · 2023-02-01T11:37:54Z

pkg/processor/sumologicschemaprocessor/README.md

+    field_attributes:
+      severity_number:
+        enabled: true
+        name: "loglevel"


Info about defaults is missing

aboguszewski-sumo

Could you add a test that checks if this is correct?

    field_attributes:
      severity_number:
        enabled: true
        name: "definitely_not_default_name"

pkg/processor/sumologicschemaprocessor/config.go

pdelewski · 2023-02-02T08:41:40Z

Could you add a test that checks if this is correct?

    field_attributes:
      severity_number:
        enabled: true
        name: "definitely_not_default_name"

@aboguszewski-sumo I updated current test using suggested name definitely_not_default_name. Will this satisfy your requirements?

aboguszewski-sumo

I overlooked one thing when I asked for that, sorry for inconvienience.

pdelewski requested a review from a team as a code owner December 19, 2022 10:56

github-actions bot added the go label Dec 19, 2022

pdelewski force-pushed the severity-number-as-field branch from 72ca7c7 to 1c9bf0c Compare December 19, 2022 11:05

aboguszewski-sumo suggested changes Dec 22, 2022

View reviewed changes

aboguszewski-sumo reviewed Dec 22, 2022

View reviewed changes

pkg/processor/sumologicschemaprocessor/log_fields_conversion_processor.go Outdated Show resolved Hide resolved

github-actions bot added the documentation Improvements or additions to documentation label Dec 22, 2022

pdelewski force-pushed the severity-number-as-field branch 4 times, most recently from edc8148 to f5996e8 Compare December 23, 2022 13:07

pdelewski closed this Dec 27, 2022

pdelewski force-pushed the severity-number-as-field branch from f5996e8 to d670949 Compare December 27, 2022 10:11

github-actions bot removed go documentation Improvements or additions to documentation labels Dec 27, 2022

pdelewski reopened this Dec 27, 2022

github-actions bot added documentation Improvements or additions to documentation go labels Dec 27, 2022

pdelewski force-pushed the severity-number-as-field branch from c33469d to e1ad3dd Compare December 27, 2022 10:19

feat: add severity number as field

73ea417

pdelewski force-pushed the severity-number-as-field branch from e1ad3dd to 73ea417 Compare December 27, 2022 10:37

pdelewski changed the title ~~WIP feat: add severity number as field~~ feat: add severity number as field Dec 27, 2022

andrzej-stencel reviewed Dec 28, 2022

View reviewed changes

Przemek Delewski added 2 commits January 3, 2023 11:42

add more fields/attributes

a89aeb1

config update

5858392

pdelewski force-pushed the severity-number-as-field branch from 6dbd3a5 to 5f8a0e1 Compare January 3, 2023 12:42

Merge branch 'main' into severity-number-as-field

74e11b1

Przemek Delewski added 3 commits January 10, 2023 14:13

fix readme

fcac479

update changelog

1ee5380

Merge branch 'main' into severity-number-as-field

8fef2c6

sumo-drosiek reviewed Jan 10, 2023

View reviewed changes

Przemek Delewski added 2 commits January 24, 2023 11:30

update readme

ba26a08

Merge branch 'main' into severity-number-as-field

61347cd

aboguszewski-sumo reviewed Jan 24, 2023

View reviewed changes

andrzej-stencel reviewed Jan 25, 2023

View reviewed changes

Przemek Delewski added 3 commits January 25, 2023 15:24

fixing bugs

ef0d1e0

add new logFieldsConversion config

2abdbb8

configuration refactoring

a2b6c4f

pdelewski force-pushed the severity-number-as-field branch from 843581b to a04f125 Compare January 25, 2023 17:07

update docs

6b88312

pdelewski force-pushed the severity-number-as-field branch from a04f125 to 6b88312 Compare January 25, 2023 17:33

Merge branch 'main' into severity-number-as-field

cda22e5

sumo-drosiek approved these changes Feb 1, 2023

View reviewed changes

aboguszewski-sumo reviewed Feb 1, 2023

View reviewed changes

aboguszewski-sumo suggested changes Feb 1, 2023

View reviewed changes

aboguszewski-sumo reviewed Feb 1, 2023

View reviewed changes

pkg/processor/sumologicschemaprocessor/config.go Outdated Show resolved Hide resolved

Przemek Delewski and others added 2 commits February 2, 2023 09:42

post-review remarks

2be879d

Merge branch 'main' into severity-number-as-field

63a3248

pdelewski requested a review from aboguszewski-sumo February 2, 2023 09:46

Merge branch 'main' into severity-number-as-field

5b7f5a4

aboguszewski-sumo approved these changes Feb 2, 2023

View reviewed changes

pdelewski merged commit 7167cf4 into main Feb 2, 2023

pdelewski deleted the severity-number-as-field branch February 2, 2023 16:04


		In order to report one of them as field, following configuration is needed:

		```json


		### Severity Attribute

		Some fields that log entries consist are not displayed as fields in Sumo Logic out of the box.

	Some fields that log entries consist are not displayed as fields in Sumo Logic out of the box.
	Some fields that log entries consist of are not displayed as fields in Sumo Logic out of the box.

feat: report attributes as fields #874

feat: report attributes as fields #874

Conversation

pdelewski commented Dec 19, 2022

swiatekm commented Dec 19, 2022

aboguszewski-sumo commented Dec 19, 2022

pdelewski commented Dec 19, 2022

aboguszewski-sumo commented Dec 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdelewski commented Jan 24, 2023

aboguszewski-sumo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrzej-stencel Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

pdelewski Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

aboguszewski-sumo Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboguszewski-sumo left a comment

Choose a reason for hiding this comment

pdelewski commented Feb 2, 2023 • edited Loading

aboguszewski-sumo left a comment

Choose a reason for hiding this comment

andrzej-stencel Jan 25, 2023 •

edited

Loading

pdelewski Jan 25, 2023 •

edited

Loading

aboguszewski-sumo Feb 1, 2023 •

edited

Loading

pdelewski commented Feb 2, 2023 •

edited

Loading