Skip to content

[Enrich] Add copy_raw_fields() method and refactor enrichers#864

Merged
valeriocos merged 2 commits intochaoss:masterfrom
imnitishng:copy_fields
Apr 30, 2020
Merged

[Enrich] Add copy_raw_fields() method and refactor enrichers#864
valeriocos merged 2 commits intochaoss:masterfrom
imnitishng:copy_fields

Conversation

@imnitishng
Copy link
Copy Markdown
Contributor

This PR removes the following redundant code snippet from enrichers

for f in self.RAW_FIELDS_COPY:
    if f in item:
        eitem[f] = item[f]
    else:
        eitem[f] = None

and replaces with new method copy_raw_fields()
Based on the discussion here #851 (comment)

Signed-off-by: Nitish Gupta imnitish.ng@gmail.com

@imnitishng imnitishng changed the title [Enrich] added method and refactored enrichers [Enrich] Add copy_raw_fields() method and refactored enrichers Apr 27, 2020
@imnitishng imnitishng changed the title [Enrich] Add copy_raw_fields() method and refactored enrichers [Enrich] Add copy_raw_fields() method and refactor enrichers Apr 27, 2020
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 27, 2020

Copy link
Copy Markdown
Member

@valeriocos valeriocos left a comment

Choose a reason for hiding this comment

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

Thank you @imnitishng for the PR.

Can you add a test to https://github.com/chaoss/grimoirelab-elk/blob/master/tests/test_enrich.py to check that the method copy_raw_fields works fine

Probably some code could be added also here: https://github.com/chaoss/grimoirelab-elk/blob/master/tests/base.py#L197 to check that the default fields are copied to each enriched document.

The code could be as the one below:

        url = self.es_con + "/" + self.enrich_index + "/_search"
        response = enrich_backend.requests.get(url, verify=False).json()

        for hit in response['hits']['hits']:
            source = hit['_source']
            ...check that the default fields have been copied

WDYT?

@imnitishng
Copy link
Copy Markdown
Contributor Author

Sure, will update the PR.

@valeriocos
Copy link
Copy Markdown
Member

Thanks @imnitishng

@imnitishng
Copy link
Copy Markdown
Contributor Author

Hi @valeriocos, I have added the tests here https://github.com/chaoss/grimoirelab-elk/blob/master/tests/test_enrich.py .
But I wonder if adding tests at https://github.com/chaoss/grimoirelab-elk/blob/master/tests/base.py#L197 might not be efficient, since in every enricher _test_raw_to_enrich() is called multiple times, causing the unnecessary overhead for checking raw fields for every test item multiple times for every backend.
I think adding a new test_raw_fields() in every backend would be a better option for a new PR.

WDYT?

@valeriocos
Copy link
Copy Markdown
Member

valeriocos commented Apr 28, 2020

Good idea @imnitishng !

Ok for the new test (the name could be test_copy_raw_fields, since we are testing that some fields are copied from the raw index to the enriched index).

I would add this new test in this PR (as a separated commit) to make sure that ELK is working properly after this modification. WDYT?

This commit adds a new method to get rid of redundant
code snippet in enrichers to copy raw fields from
raw item to rich item.

Signed-off-by: Nitish Gupta <imnitish.ng@gmail.com>
@imnitishng imnitishng force-pushed the copy_fields branch 2 times, most recently from 79f6ffb to ed27740 Compare April 29, 2020 11:21
@imnitishng
Copy link
Copy Markdown
Contributor Author

I have updated the tests @valeriocos, please have a look whenever you're free.

Copy link
Copy Markdown
Member

@valeriocos valeriocos left a comment

Choose a reason for hiding this comment

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

Thank you @imnitishng for the PR. I left a minor comment

if attribute in item:
self.assertEqual(item[attribute], eitem[attribute])
else:
self.assertEqual(eitem[attribute], None)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be assertIsNone

This commit adds tests for the `copy_raw_fields()`
method for all ELK backends to check if all the
raw attributes are copied properly.

Signed-off-by: Nitish Gupta <imnitish.ng@gmail.com>
Copy link
Copy Markdown
Member

@valeriocos valeriocos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @imnitishng

@valeriocos valeriocos merged commit 837d5b2 into chaoss:master Apr 30, 2020
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.

3 participants