Skip to content

Migrate obs-infraobs-integrations to package-spec v3 #6 #8291

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.

Sign up for GitHub

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

Closed
shmsr wants to merge 2 commits into from
Closed

Migrate obs-infraobs-integrations to package-spec v3 #6 #8291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
398d2aa
[statsd_input]: migrate to package-spec v3
shmsr Oct 25, 2023
39071f9
[sql_input]: migrate to package-spec v3
shmsr Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/sql_input/_dev/build/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
dependencies:
ecs:
reference: git@v8.7.0

5 changes: 5 additions & 0 deletions packages/sql_input/changelog.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
# newer versions go on top
- version: 0.5.0
changes:
- description: Update the package format_version to 3.0.0.
type: enhancement
link: https://github.com/elastic/integrations/pull/8203
- version: "0.4.0"
changes:
- description: Add `condition` and `processors` settings.
Expand Down
1 change: 1 addition & 0 deletions packages/sql_input/fields/input.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@
- name: metrics.*
description: Default mapping for metric fields. You need to add your own custom mappings when storing other kinds of information.
type: object
object_type: keyword
Copy link
Contributor

@tommyers-elastic tommyers-elastic Oct 25, 2023

Choose a reason for hiding this comment

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

this is a weird one - we don't want to put a type here at all, we want to keep the mapping dynamic for all subfields in metrics which would be something like

name: metrics
type: object
dynamic: true

the only problem with this is that we get failing validations here if we include any metrics.* fields in sample_event.json.

removing the values from the sample event doesn't seem right, because the mapping supports them. i wonder what the right course of action is here?

Copy link
Contributor

@lalit-satapathy lalit-satapathy Oct 25, 2023

Choose a reason for hiding this comment

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

Isn't object_type a must now, as per elastic/package-spec#628 ?

Copy link
Contributor

@tommyers-elastic tommyers-elastic Oct 25, 2023

Choose a reason for hiding this comment

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

the intention there is to remove ambiguous mappings, but in this case we want the mapping to be ambiguous.

there's no sensible value we can set here for object_type.

Copy link
Member

@ishleenk17 ishleenk17 Oct 25, 2023

Choose a reason for hiding this comment

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

This will be true for most Input Packages, I suppose. Since the mapping has to be dynamic.
The metrics.* was added to have some mapping for the system tests to run to have the sample event generated

Copy link
Contributor

@tommyers-elastic tommyers-elastic Oct 30, 2023

Choose a reason for hiding this comment

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

i guess we have two options here then - we can either remove the metrics.* mapping entirely, and ensure we document that we expect users to define custom mappings, or we use some sensible default numerical value, like double.

i'm happy with either to get us unblocked. what do you all think?

Copy link
Member

@ebeahan ebeahan Dec 1, 2023

Choose a reason for hiding this comment

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

The windows package also has this same challenge, and it's preventing an upgrade to a newer spec. Any updates on how to handle this blocker?

Copy link
Contributor

@taylor-swanson taylor-swanson Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I started working on the windows package before I noticed Eric's comments about it and of course ran into the same issue. I'm also running into this same mapping issue with https://github.com/elastic/security-team/issues/7388 (moving to package-spec 3.0.2 includes this validation and a number of integrations are failing said validation).

Echoing Eric's comment above, any updates/ideas on how to handle this blocker? We can't remove the mappings, since our example data includes these fields, and in at least one integration, the sub-fields for a field span multiple types, so object_type isn't going to work there.

Copy link
Member Author

@shmsr shmsr Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Apologies, @ebeahan, as I seem to have missed your comment. As for you, @taylor-swanson, we have not yet reached a conclusion, which is why the PR is still open. Although we did discuss it in our team meeting once (points that @tommyers-elastic suggested above), we were unable to come to a decision at that time. I will make an effort to communicate with my team again regarding this matter since the PR is still stuck on it.

Copy link
Member

@jsoriano jsoriano Jan 10, 2024

Choose a reason for hiding this comment

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

For this case, if the problem is that the test is generating fields under this untyped object, maybe we could try to configure the test so it uses the typed fields. Or if we want to explicitly test these untyped fields, maybe we need to add support to elastic-package test to include custom mappings.

In general, if these objects are included only for testing and documentation purposes, and it is expected for users to provide custom mappings if they want to use these fields, something you could try is to use index: false. This is also an option if you want these fields to be in the source just in case, but they are not expected to be directly used.
In principle this would be allowed by the spec, but these subfields won't be indexed unless some custom mapping is provided.

    - name: metrics.*
      description: Default mapping for metric fields. You need to add your own custom mappings when storing other kinds of information.
      type: object
      index: false

11 changes: 7 additions & 4 deletions packages/sql_input/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
format_version: 2.0.0
format_version: "3.0.0"
name: sql
title: "SQL Input"
version: "0.4.0"
version: "0.5.0"
description: "Collects Metrics by Quering on SQL Databases"
type: input
categories:
- custom
conditions:
kibana.version: "^8.8.0"
elastic.subscription: "basic"
kibana:
version: "^8.8.0"
elastic:
subscription: "basic"
icons:
- src: /img/sql-server-icon.svg
title: SQL logo
Expand Down Expand Up @@ -73,3 +75,4 @@ policy_templates:
Processors are used to reduce the number of fields in the exported event or to enhance the event with metadata. This executes in the agent before the events are shipped. See [Processors](https://www.elastic.co/guide/en/fleet/current/elastic-agent-processor-configuration.html) for details.
owner:
github: elastic/obs-infraobs-integrations
type: elastic
1 change: 0 additions & 1 deletion packages/statsd_input/_dev/build/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
dependencies:
ecs:
reference: git@v8.7.0

2 changes: 1 addition & 1 deletion packages/statsd_input/_dev/test/system/test-default-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ service_notify_signal: SIGHUP
vars:
listen_address: 0.0.0.0
listen_port: 8125
data_stream.dataset: statsd_input.statsd
"data_stream.dataset": statsd_input.statsd
assert:
hit_count: 3
5 changes: 5 additions & 0 deletions packages/statsd_input/changelog.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
# newer versions go on top
- version: 0.3.0
changes:
- description: Update the package format_version to 3.0.0.
type: enhancement
link: https://github.com/elastic/integrations/pull/8203
- version: "0.2.3"
changes:
- description: Improve documentation for the package.
Expand Down
1 change: 1 addition & 0 deletions packages/statsd_input/fields/input.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@
- name: statsd.*.*
description: Default mapping for metric fields. You need to add your own custom mappings when storing other kinds of information.
type: object
object_type: keyword
11 changes: 7 additions & 4 deletions packages/statsd_input/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
format_version: 2.0.0
format_version: "3.0.0"
name: statsd_input
title: StatsD Input
version: "0.2.3"
version: "0.3.0"
description: StatsD Input Package
type: input
categories:
- observability
conditions:
kibana.version: "^8.8.0"
elastic.subscription: "basic"
kibana:
version: "^8.8.0"
elastic:
subscription: "basic"
icons:
- src: /img/statsd.svg
title: statsd
Expand Down Expand Up @@ -40,3 +42,4 @@ policy_templates:
default: 8125
owner:
github: elastic/obs-infraobs-integrations
type: elastic