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

[DSET-4037] [DSET-4014] [DSET-4034] [DSET-4034] [DSET-4098] Dataset log exporter improvements #3

Merged
merged 13 commits into from
Jun 26, 2023

Conversation

tomaz-s1
Copy link

@tomaz-s1 tomaz-s1 commented Jun 20, 2023

This pull request contains various small improvements, changes and fixes for the DataSet Log exporter.

I bundled all the changes together since they are all on the small side and part of the general exporter clean up and improvements work.

1. Remove OtelExporter - Log - prefix from all the DataSet event message fields - DSET-4014

This prefix is not needed nor desired.

2. Correctly handle log record severity / log level - DSET-4037

I updated the code to correctly handle and map from OTel LogRecord severity to DataSet event severity (logic is described in the code comments).

I also removed now redundant and unnecessary event severity.number and severity.text attributes from the event.

3. Remove redundant flags and flags.is_sampled attribute - DSET-4034

Remove unnecessary and redundant flags and flags.is_sampled attribute.

4. Update metadata file to include distributions field - DSET-4098

Title is self explanatory.

tomaz-s1 added 2 commits June 20, 2023 08:55
severity to DataSet severity.

Also remove "severity.text" and "severity.number" field from the event
since it's redundant - we already have event severity (sev) field value.
@tomaz-s1 tomaz-s1 requested a review from zdaratom-s1 June 21, 2023 08:44
information which is not already available (events which are sampled
will already be ingested and visible in the ui so flags.is_sampled is
redundant).
@tomaz-s1 tomaz-s1 changed the title [DSET-4037] [DSET-4014] Dataset log exporter improvements [DSET-4037] [DSET-4014] [DSET-4034] Dataset log exporter improvements Jun 21, 2023
@tomaz-s1 tomaz-s1 changed the title [DSET-4037] [DSET-4014] [DSET-4034] Dataset log exporter improvements [DSET-4037] [DSET-4014] [DSET-4034] [DSET-4034] Dataset log exporter improvements Jun 21, 2023
exporter/datasetexporter/logs_exporter.go Outdated Show resolved Hide resolved
exporter/datasetexporter/logs_exporter.go Outdated Show resolved Hide resolved
exporter/datasetexporter/logs_exporter.go Outdated Show resolved Hide resolved
exporter/datasetexporter/logs_exporter.go Outdated Show resolved Hide resolved
exporter/datasetexporter/logs_exporter_test.go Outdated Show resolved Hide resolved
exporter/datasetexporter/logs_exporter.go Outdated Show resolved Hide resolved
@tomaz-s1 tomaz-s1 changed the title [DSET-4037] [DSET-4014] [DSET-4034] [DSET-4034] Dataset log exporter improvements [DSET-4037] [DSET-4014] [DSET-4034] [DSET-4034] [DSET-4098] Dataset log exporter improvements Jun 22, 2023
@tomaz-s1
Copy link
Author

@zdaratom I pushed some commits which I believe should address all the feedback we agreed on (I didn't push map change yet + possible parameterized tests change since I had a bit different opinion on those and we didn't agree on the approach yet).

As I said before, I still plan to push one other changes in the near future (better / correct attribute name and value handling - aka getting rid non DataSet ideomatic approach of parameter.name and parameter.value).

@tomaz-s1 tomaz-s1 marked this pull request as ready for review June 23, 2023 10:34
@tomaz-s1
Copy link
Author

As discussed on Slack, I will work on that additional change (correct attribute field name and value handling in a separate PR in the very near future.

This way I can try and submit this PR upstream for review today and get CLA, etc. sorted out as soon as possible.

@tomaz-s1 tomaz-s1 force-pushed the dataset_log_exporter_improvements branch from 2758a0a to 21c07be Compare June 23, 2023 12:07
Copy link
Collaborator

@zdaratom-s1 zdaratom-s1 left a comment

Choose a reason for hiding this comment

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

exporter/datasetexporter/logs_exporter.go Outdated Show resolved Hide resolved
exporter/datasetexporter/logs_exporter_test.go Outdated Show resolved Hide resolved
tomaz-s1 added 2 commits June 23, 2023 15:44
functions. Also move functionality for making a test record into a
utility function.
@tomaz-s1
Copy link
Author

@zdaratom "map" prefix should be addressed in 211db7e and test functions refactor / split in 656cc5f.

@zdaratom-s1
Copy link
Collaborator

@tomaz-s1
Copy link
Author

@zdaratom Thanks.

Yeah, I wanted to add changelog entry, but I saw it needs to reference github issue / pr and that won't be available until I open upstream PR. So I plan to do that once upstream PR is opened.

@zdaratom-s1
Copy link
Collaborator

I have referenced previously created issue [20660] so far it was enough

@tomaz-s1 tomaz-s1 force-pushed the dataset_log_exporter_improvements branch from 138c36e to 401041d Compare June 23, 2023 14:31
@tomaz-s1 tomaz-s1 merged commit 292122c into datasetexporter-latest Jun 26, 2023
@tomaz-s1 tomaz-s1 deleted the dataset_log_exporter_improvements branch June 26, 2023 07:47
@tomaz-s1 tomaz-s1 restored the dataset_log_exporter_improvements branch June 26, 2023 07:57
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.

2 participants