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

[Metricbeat] Add a switch to the driver definition on SQL module to use pretty names #17378

Merged

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Mar 31, 2020

What does this PR do?

SQL module directly passes the driver name to the sqlx library. This must match the library name used in Metricbeat: mysql for MySQL, postgresql for PostgreSQL but godror for Oracle which is ugly in terms of UX.

Why is it important?

Docs shouldn't fix UX issues so this PR fixes this in code.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

  • Launch Oracle Docker Image
docker run --name oracle -d -p 1521:1521 store/oracle/database-enterprise:12.2.0.1
# Module: sql
# Docs: https://www.elastic.co/guide/en/beats/metricbeat/7.7/metricbeat-module-sql.html

- module: sql
  metricsets:
    - query
  period: 10s
  hosts: ["oracle://sys:Oradoc_db1@172.17.0.2:1521/ORCLPDB1.localdomain?sysdba=1"]

  driver: "oracle"
  sql_query: "SELECT TABLESPACE_NAME, TABLESPACE_SIZE, ALLOCATED_SPACE, FREE_SPACE FROM DBA_TEMP_FREE_SPACE"
  sql_response_format: table
  • An event is generated with the driver name oracle

@sayden sayden added bug Metricbeat Metricbeat Team:Services (Deprecated) Label for the former Integrations-Services team labels Mar 31, 2020
@sayden sayden self-assigned this Mar 31, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-services (Team:Services)

@ycombinator
Copy link
Contributor

I like what this PR is trying to do, which is to hide the implementation (the driver library name) from the interface (the pretty name). This also gives us the option of swapping out a driver library for another one while not requiring any changes from users.

The way this PR is currently implemented, however, a user would be able to specify either oracle or godror as the pretty name. Is this what we want? Or should we keep a map[string][string] of pretty names => implementation driver library names and error out if the pretty name key isn't found in the map?

@sayden
Copy link
Contributor Author

sayden commented Apr 7, 2020

I thought about doing a map but then if we add a new SQL-based module (ie Clickhouse) we'll need to update the map (which is going to be out of the context of the "other sql module" PR).

The alternative is to use a map to names we know which are "ugly" only like godror but leave the possibility to use any "unknown" pretty name. WDYT?

@ycombinator
Copy link
Contributor

Fair point, @sayden. Let's keep this as-is then — the user can specify the "pretty" name for the godror module or the actual name of the driver itself.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM. WFG.

@sayden sayden changed the title [Metricbeat] Add a switch to the driver definition on SQL module [Metricbeat] Add a switch to the driver definition on SQL module to use pretty names Apr 14, 2020
@sayden sayden merged commit cddf0c1 into elastic:master Apr 14, 2020
sayden added a commit to sayden/beats that referenced this pull request Apr 14, 2020
@sayden sayden added the v7.8.0 label Apr 14, 2020
sayden added a commit that referenced this pull request May 5, 2020
…finition on SQL module to use pretty names (#17708)
@sayden sayden deleted the bugfix/xp/mb/sql/add-switch-on-driver-definition branch August 25, 2022 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Metricbeat Metricbeat Team:Services (Deprecated) Label for the former Integrations-Services team v7.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants