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

[Fleet][Kafka Output] Tweaks for Kafka UI/API #162875

Merged
merged 41 commits into from
Aug 9, 2023

Conversation

szwarckonrad
Copy link
Contributor

@szwarckonrad szwarckonrad commented Aug 1, 2023

  1. Removed Kerberos option from auth type selection
  2. Used EuiFieldPassword component for password input
    State:
    Screenshot 2023-08-01 at 11 16 30
  3. Added None authentication option. This will save auth_type as none and won't pass usernamer, password nor ssl to the SO.
    Screenshot 2023-08-02 at 15 59 08
  4. Renamed broker_ack_reliability to required_acks in UI and API. Changed value type to 1 || 0 || -1
  5. Renamed broker_buffer_size to channel_buffer_size in UI and API.
  6. Temporarily disabled password field encryption.
  7. partition key stored in yaml follows proper requirements
  8. topics key stored in yaml follows proper requirements

@kevinlog kevinlog added the ci:cloud-deploy Create or update a Cloud deployment label Aug 2, 2023
@szwarckonrad szwarckonrad added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Aug 2, 2023
@kilfoyle
Copy link
Contributor

kilfoyle commented Aug 2, 2023

Hi @szwarckonrad! I'm working on a docs PR for these UI changes.

I have a couple of suggestions if you don't mind:

  1. It might be good to have the Kafka help text match what we have for the Logstash output settings UI.
    Kafka output settings UI:
    kafka-ui-text
    Logstash output settings UI:
    logstash-ui-text
    Could we change the Kafka UI text to Specify the URLs that your agents will use to connect to Kafka. Learn more?

  2. Once merged, my docs PR will create individual pages for the Logstash output settings and the Kafka output settings. For those "Learn more" links, would it be possible to have them point to <base-path>/ls-output-settings.html and <base-path>/kafka-output-settings.html, respectively, instead of the main UI settings page (fleet-settings.html)?

Thanks!

@kevinlog
Copy link
Contributor

kevinlog commented Aug 2, 2023

@szwarckonrad Thank you for putting together these changes, it's a massive help!

I was chatting with @faec and there are a couple more changes in the conversion to elastic-agent.yml that we should tweak:

specifically partition and topics

Partition

We need to follow the rules here: https://www.elastic.co/guide/en/beats/filebeat/current/kafka-output.html#_partition

Right now partition is the following:

random
    partition: random
    random:
      group_events: 1

So this should look like

    partition:
      random:
        group_events: 1

partition should be an object, i.e. partition.random.group_events: 1

round_robin

Similarly, for round_robin

Right now it looks like

    partition: round_robin
    round_robin:
      group_events: 1

it should be

    partition:
       round_robin:
         group_events: 1
hash

for hash:

    partition:
      hash:
        hash: 'asdasd,asdasd'
yaml generation bug?

I also think there might be a bug here:

When I change the partition a few times, it looks like hash, round_robin, and random get appended to the yaml. We should just be replacing the entire partition field each time depending on the user selection.

I'm seeing this in the yaml, we should follow the format above and ensure we just have partition.{strategy}.{group_events | hash}

    partition: hash
    random:
      group_events: 1
    round_robin:
      group_events: 1
    hash:
      hash: 'asdasd,asdasd'

Topics

We need to follow the rules here: https://www.elastic.co/guide/en/beats/filebeat/current/kafka-output.html#topic-option-kafka

For topics, I'm seeing the following

  topics:
    - topic: asd
      when:
        type: contains
        condition: sdf
    - topic: asd

We should actually see this as:

topics:
  - topic: asd
    when:
      contains:
        message: sdf
  - topic: asd
  • type: contains should a field under when: that is just called contains.
  • condition: sdf should be message: sdf and be nested under when.

@szwarckonrad
Copy link
Contributor Author

@kilfoyle I changed the copy, however, hrefs come from kbn-doc-links package and we use them, following logstash example, like that href={docLinks.links.fleet.settings}. My guess would be that change there is required so that we can consume these links for both logstash and kafka. CC @kevinlog
Screenshot 2023-08-03 at 14 43 40

@kilfoyle
Copy link
Contributor

kilfoyle commented Aug 3, 2023

I changed the copy, however, hrefs come from kbn-doc-links package and we use them, following logstash example, like that href={docLinks.links.fleet.settings}. My guess would be that change there is required so that we can consume these links for both logstash and kafka.

Nice. Thanks a lot for fixing the copy, Konrad! Now that I understand how the hrefs are handled, I'll wait for my docs PR to merge (so that the URLs exist) and then I'll open a separate issue or PR to have the hrefs updated.

@cmacknz
Copy link
Member

cmacknz commented Aug 3, 2023

Can we add #163093 to the changes in this PR as well?

We should remove the Channel Buffer Size configuration, it's an advanced option so can go in the Advanced YAML parameters box when needed.

@kevinlog kevinlog added ci:cloud-redeploy Always create a new Cloud deployment ci:cloud-deploy Create or update a Cloud deployment and removed ci:cloud-deploy Create or update a Cloud deployment labels Aug 3, 2023
@kevinlog
Copy link
Contributor

kevinlog commented Aug 3, 2023

@elasticmachine merge upstream

@kevinlog
Copy link
Contributor

kevinlog commented Aug 3, 2023

@szwarckonrad a few more changes after some additional testing.

Remove channel buffer size

We should remove the channel buffer size field entirely.

#163093

One more tweak to topics

Right now, we're getting the following:

    topics:
      - topic: asd
        contains:
          message: asd
      - topic: asd
        range:
          message: asd
      - topic: sad

We should be getting:

    topics:
      - topic: asd
        when:
          contains:
            message: asd
      - topic: asd
        when:
          range:
            message: asd
      - topic: sad

Note that we should have when above the conditions (i.e. contains, range).

Hosts field should accept host:port

The hosts field should actually only support host:port - we should not include the protocol

for instance:

  • http://localhost:1234
  • localhost:1234

Right now the validation is requiring that we enter a URL. We should just be enforcing host:port.

The client ID field should not have any spaces

The Client ID field should NOT have any spaces. We should change the default to Elastic (one word). Also, we should add validation with this regex ^[A-Za-z0-9._-]+$

@szwarckonrad szwarckonrad requested a review from a team as a code owner August 8, 2023 13:43
@@ -229,6 +232,29 @@ const getSavedObjectTypes = (): { [key: string]: SavedObjectsType } => ({
broker_timeout: { type: 'integer' },
broker_ack_reliability: { type: 'text' },
broker_buffer_size: { type: 'integer' },
required_acks: { type: 'integer' },
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add index: false to all of those kafka fields as it seems to me that we will not search on that

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Aug 8, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@@ -84,13 +84,13 @@ export const OutputFormKafkaSection: React.FunctionComponent<Props> = (props) =>
helpText={
<FormattedMessage
id="xpack.fleet.settings.editOutputFlyout.kafkaHostsInputDescription"
defaultMessage="Specify the URLs that your agents will use to connect to Kafka. For more information, see the {guideLink}."
defaultMessage="Specify the URLs that your agents will use to connect to Kafka. {guideLink}."
values={{
guideLink: (
<EuiLink href={docLinks.links.fleet.settings} target="_blank" external>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to update this to a Kafka specific doc link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

could we add a unit test on the migration?

Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

LGTM, please check comments.

Copy link
Contributor

@gsoldevila gsoldevila left a comment

Choose a reason for hiding this comment

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

LGTM changes in the ingest-outputs mappings are compatible (non-breaking).

@nchaulet nchaulet self-requested a review August 9, 2023 13:37
@nchaulet
Copy link
Member

nchaulet commented Aug 9, 2023

I think there is one part we may been missing for Kafka output (this should probably be a follow up PR) it's the whole preconfiguration thing to be able to define a kafka output in kibana config file, probably need to update that function and check the schema support it

function isPreconfiguredOutputDifferentFromCurrent(

@szwarckonrad szwarckonrad enabled auto-merge (squash) August 9, 2023 14:59
@kibana-ci
Copy link
Collaborator

kibana-ci commented Aug 9, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Defend Workflows Cypress Tests #1 / Endpoint Policy Response from Fleet Agent Details page should display policy response with errors should display policy response with errors
  • [job] [logs] Defend Workflows Endpoint Cypress Tests #5 / Isolate command From cases should isolate and release host should isolate and release host
  • [job] [logs] FTR Configs #52 / ObservabilityApp Observability Rules page Rules table shows the rules table

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 1012.8KB 1016.1KB +3.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 136.5KB 136.7KB +163.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@szwarckonrad szwarckonrad merged commit e2b10cb into elastic:main Aug 9, 2023
bryce-b pushed a commit to bryce-b/kibana that referenced this pull request Aug 9, 2023
1. Removed Kerberos option from auth type selection
2. Used EuiFieldPassword component for password input
State:
![Screenshot 2023-08-01 at 11 16
30](https://github.com/elastic/kibana/assets/29123534/3ef0b3f8-c98b-42d0-9645-fdb6437859f5)
3. Added `None` authentication option. This will save `auth_type` as
`none` and won't pass `usernamer`, `password` nor `ssl` to the SO.
![Screenshot 2023-08-02 at 15 59
08](https://github.com/elastic/kibana/assets/29123534/9bbe9ce7-2749-484f-833d-72d1c9e5ee46)
4. Renamed `broker_ack_reliability` to `required_acks` in UI and API.
Changed value type to `1 || 0 || -1`
5. Renamed `broker_buffer_size` to `channel_buffer_size` in UI and API.
6. Temporarily disabled `password` field encryption.
7. `partition` key stored in yaml follows proper requirements
8. `topics` key stored in yaml follows proper requirements

---------

Co-authored-by: kibanamachine <[email protected]>
szwarckonrad added a commit that referenced this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment ci:cloud-redeploy Always create a new Cloud deployment release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.