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

ECS: support composite region_iso_code #206

Merged
merged 5 commits into from
Jan 26, 2022

Conversation

yaauie
Copy link
Contributor

@yaauie yaauie commented Jan 25, 2022

Adds support for ECS's composite region_iso_code, which combines the ISO 2-char country code with the ISO 2-char region code.

When running in ECS mode with these changes, we prefer the region_iso_code to the non-ECS region_code.

static final EnumSet<Fields> DEFAULT_ECS_CITY_FIELDS;
static {
DEFAULT_ECS_CITY_FIELDS = EnumSet.copyOf(DEFAULT_CITY_FIELDS);
DEFAULT_ECS_CITY_FIELDS.remove(REGION_CODE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is perhaps a breaking change from a certain perspective, for a user who was relying on the non-ECS [geo][region_code] being populated in ECS mode. I'm inclined to call this a bugfix, since users have to explicitly opt into ECS to hit this behaviour.

But if we want to continue populating [geo][region_code] in ECS mode, this change and a small change to the specs would suffice:

Suggested change
DEFAULT_ECS_CITY_FIELDS.remove(REGION_CODE);

Copy link
Contributor

Choose a reason for hiding this comment

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

With ECS default on in 8.x, this is a breaking change. Users do not have the same data of region_code or replacement. Thinking to have both region_code and region_iso_code, but in some sense, the data of region_code is also a kind of iso code. Having both seems to be confusing. +1 to the breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With ECS default on in 8.x, this is a breaking change

A user upgrading from Logstash 7 to Logstash 8 should be expecting to be consuming breaking changes, and the path to ECS provides appropriate guidance on how to keep a plugin in non-ECS mode (hint: set pipeline.ecs_compatibility: disabled for the pipeline to lock in 7.x behaviour in all its plugins).

It is a little breaking for users who already opt into ECS for the pipeline or an instance of this plugin, but it is also a bugfix since users who are already going out of their way to get ECS have a reasonable expectation that the result will be ECS (which [geo][region_code] is not).

Comment on lines 49 to 58
if ecs_select.active_mode == :disabled
expect( event.get "[#{target}][country_code3]" ).to eq 'US'
expect( event.get "[#{target}][region_code]" ).to eq 'MA'
expect( event.get "[#{target}][region_iso_code]" ).to be_nil
else
expect( event.get "[#{target}][geo][country_code3]" ).to be_nil
expect( event.get "[#{target}][country_code3]" ).to be_nil
expect( event.get "[#{target}][geo][region_iso_code]" ).to eq 'US-MA'
expect( event.get "[#{target}][region_code]" ).to be_nil
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to keep populating the non-ECS [geo][region_code], this change is also needed:

Suggested change
if ecs_select.active_mode == :disabled
expect( event.get "[#{target}][country_code3]" ).to eq 'US'
expect( event.get "[#{target}][region_code]" ).to eq 'MA'
expect( event.get "[#{target}][region_iso_code]" ).to be_nil
else
expect( event.get "[#{target}][geo][country_code3]" ).to be_nil
expect( event.get "[#{target}][country_code3]" ).to be_nil
expect( event.get "[#{target}][geo][region_iso_code]" ).to eq 'US-MA'
expect( event.get "[#{target}][region_code]" ).to be_nil
end
if ecs_select.active_mode == :disabled
expect( event.get "[#{target}][country_code3]" ).to eq 'US'
expect( event.get "[#{target}][region_code]" ).to eq 'MA'
expect( event.get "[#{target}][region_iso_code]" ).to be_nil
else
expect( event.get "[#{target}][geo][country_code3]" ).to be_nil
expect( event.get "[#{target}][country_code3]" ).to be_nil
expect( event.get "[#{target}][geo][region_iso_code]" ).to eq 'US-MA'
expect( event.get "[#{target}][geo][region_code]" ).to eq 'MA'
end

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Nice improvements. Builds cleanly and the table renders as it should. I left some suggestion inline related to readability, translation, tense, subject/verb agreement, etc. Please let me know if you would like to discuss.

Sounds like you're on a tight timeline. If you want to proceed with what you have, you have my LGTM. I can do a follow up PR with these tweaks.

docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
docs/index.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
@karenzone karenzone changed the title ECS: supoprt composite region_iso_code ECS: support composite region_iso_code Jan 25, 2022
Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kaisecheng kaisecheng left a comment

Choose a reason for hiding this comment

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

Thanks for adding the missing ECS doc 💮 I support the change of removing [geo][region_code] in ECS mode. Left two comments on the code side

static final EnumSet<Fields> DEFAULT_ECS_CITY_FIELDS;
static {
DEFAULT_ECS_CITY_FIELDS = EnumSet.copyOf(DEFAULT_CITY_FIELDS);
DEFAULT_ECS_CITY_FIELDS.remove(REGION_CODE);
Copy link
Contributor

Choose a reason for hiding this comment

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

With ECS default on in 8.x, this is a breaking change. Users do not have the same data of region_code or replacement. Thinking to have both region_code and region_iso_code, but in some sense, the data of region_code is also a kind of iso code. Having both seems to be confusing. +1 to the breaking change.

CHANGELOG.md Outdated Show resolved Hide resolved
@yaauie yaauie merged commit feeb56b into logstash-plugins:main Jan 26, 2022
@yaauie yaauie deleted the ecs-region_iso_code branch January 26, 2022 06:07
@kaisecheng kaisecheng mentioned this pull request Jun 29, 2022
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.

4 participants