ECS: support composite region_iso_code

[yaauie]

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.

[yaauie]

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);

[kaisecheng]

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.

[yaauie]

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).

[yaauie]

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

[karenzone]

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.

[karenzone]

LGTM

[kaisecheng]

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

[kaisecheng]

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.