elastic / apm

Elastic Application Performance Monitoring - resources and general issue tracking for Elastic APM.
https://www.elastic.co/apm
Apache License 2.0
385 stars 114 forks source link

[Agents] Oracle SQL parsing improvements #108

Open axw opened 5 years ago

axw commented 5 years ago

Description of the issue

In https://github.com/elastic/apm-agent-java/pull/696 there are several further improvements made to SQL parsing which are relevant to all agents, relating to parsing Oracle SQL:

Proposed solution

MERGE statements

For a statement like MERGE INTO <target> USING <source> ..., we would set the span name to MERGE INTO <target>.

Database links

Anywhere we extract a table name (e.g. in SELECT, UPDATE, INSERT, etc.), we should also look for a database link in the form of two consecutive tokens: a @ character, followed by an identifier (possibly double-quoted). If we find this sequence immediately following a table name, we'll assume it is a database link.

The database link should not be recorded in span names, but instead recorded as a separate field per the proposal in https://github.com/elastic/apm/issues/107. This would permit aggregation of all like queries regardless of destination database, in addition to a breakdown by destination database. Until that proposal is accepted, we will not record the link anywhere.

Acceptance criteria

Additional JSON test cases, from https://github.com/elastic/apm-agent-java/pull/696
  {
    "input": "MERGE INTO TEST USING (SELECT * FROM DUAL) SOURCE on (TEST.X=SOURCE.DUMMY) WHEN NOT MATCHED THEN INSERT VALUES(SOURCE.DUMMY)",
    "output": "MERGE INTO TEST"
  },
  {
    "input": "MERGE INTO TEST ALIAS USING (SELECT * FROM DUAL) SOURCE on (TEST.X=SOURCE.DUMMY) WHEN NOT MATCHED THEN INSERT VALUES(SOURCE.DUMMY)",
    "output": "MERGE INTO TEST"
  },
  {
    "input": "MERGE INTO SCHEMA.TEST USING (SELECT * FROM DUAL) SOURCE on (TEST.X=SOURCE.DUMMY) WHEN NOT MATCHED THEN INSERT VALUES(SOURCE.DUMMY)",
    "output": "MERGE INTO SCHEMA.TEST"
  },
  {
    "input": "MERGE INTO SCHEMA.TEST@DBLINK USING (SELECT * FROM DUAL) SOURCE on (TEST.X=SOURCE.DUMMY) WHEN NOT MATCHED THEN INSERT VALUES(SOURCE.DUMMY)",
    "output": "MERGE INTO SCHEMA.TEST"
  },
  {
    "input": "MERGE INTO SCHEMA.TEST@\"DBLINK.FQDN.COM\" USING (SELECT * FROM DUAL) SOURCE on (TEST.X=SOURCE.DUMMY) WHEN NOT MATCHED THEN INSERT VALUES(SOURCE.DUMMY)",
    "output": "MERGE INTO SCHEMA.TEST"
  },
  {
    "input": "MERGE INTO SCHEMA.TEST@\"DBLINK.FQDN.COM@USER\" USING (SELECT * FROM DUAL) SOURCE on (TEST.X=SOURCE.DUMMY) WHEN NOT MATCHED THEN INSERT VALUES(SOURCE.DUMMY)",
    "output": "MERGE INTO SCHEMA.TEST"
  },
  {
    "input": "MERGE",
    "output": "MERGE"
  },

What we are voting on

@elastic/apm-agent-devs Tick the N/A box if your agent isn't affected (e.g. there is no Oracle database driver). Otherwise, please create and link to an issue or PR and tick "Yes". If you have concerns about the proposed solution, let's discuss.

Vote

Agent Yes No Indifferent N/A Link to agent issue
.NET
  • [x]
  • [ ]
  • [ ]
  • [ ]
https://github.com/elastic/apm-agent-dotnet/issues/357
Go
  • [ ]
  • [ ]
  • [ ]
  • [ ]
Java
  • [x]
  • [ ]
  • [ ]
  • [ ]
https://github.com/elastic/apm-agent-java/pull/696
Node.js
  • [ ]
  • [ ]
  • [ ]
  • [x]
Python
  • [ ]
  • [ ]
  • [ ]
  • [ ]
Ruby
  • [ ]
  • [ ]
  • [x]
  • [ ]
SergeyKleyman commented 5 years ago

Otherwise, please create and link to an issue or PR and tick "Yes". If you have concerns about the proposed solution, let's discuss.

@axw When can we choose "Indifferent"?

axw commented 5 years ago

@SergeyKleyman (and @watson since you ticked) I probably shouldn't have left that option in, but I'll make up a reason now:

Tick "Indifferent" if you're happy for others to decide. We'll want to make this consistent across agents, so it'll still affect you unless it's really "N/A".

watson commented 5 years ago

I meant to tick N/A, as we don't support Oracle in the Node.js agent. Was just off by one. I'll update the checkbox