Instrument usage of ExecutableNormalizedOperationFactory.createExecutableNormalizedOperation #4045
Conversation
timward60
force-pushed
the
timward-instrument-eno
branch
from
74e7ed3 to
9553a10
Compare
…ableNormalizedOperation
timward60
force-pushed
the
timward-instrument-eno
branch
from
9553a10 to
d0b2634
Compare
src/main/java/graphql/execution/instrumentation/Instrumentation.java
Outdated
Show resolved
Hide resolved
timward60
force-pushed
the
timward-instrument-eno
branch
from
f36ea22 to
8a4c715
Compare
timward60
force-pushed
the
timward-instrument-eno
branch
from
8a4c715 to
6ac9b17
Compare
src/main/java/graphql/execution/instrumentation/Instrumentation.java
Outdated
Show resolved
Hide resolved
...a/graphql/execution/instrumentation/parameters/InstrumentationParsedNormalizedOperation.java
Outdated
Show resolved
Hide resolved
| @@ -23,7 +23,7 @@ | |||
| * An operation consists of a list of {@link ExecutableNormalizedField}s in a parent child hierarchy | |||
| */ | |||
| @PublicApi | |||
| public class ExecutableNormalizedOperation { | |||
| public class ExecutableNormalizedOperation implements ParsedNormalizedOperation { | |||
There was a problem hiding this comment.
Ahh you are going for a common interface - again I dont think its "parsed" - this is not parsing - parsing has a very specific name in this code base - eg text -> AST elements
There was a problem hiding this comment.
Yes this bit I expected some discussion. Possibly I should rename the new normalized operation implementation classes to something else then use NormalizedOperation as the interface name, otherwise if there is a better interface name then we can change to that. Let me know what you think?
There was a problem hiding this comment.
For reference here is the follow up PR (WIP) to enable a config to switch between ExecutableNormalizedOperation and NormalizedOperation. #4047
There was a problem hiding this comment.
Ahh didnt see your replies when I made more naming suggestions
There was a problem hiding this comment.
name storming
NormalizedOperation + ExecutableNormalizedOperation
What is the common name here?
NormalOperation
GraphQlOperation
GraphQlNormalizedOperation
Leaning towards GraphQlNormalizedOperation.
There was a problem hiding this comment.
@bbakerman and I had a chat and thinking more about it I am not 100% sure we really want an interface and two different implementations.
Not only the naming makes it clear that this is very hard to distinguish ... maybe we really want to have just NormalizedField which is a combination of ENF and the current NF.
There was a problem hiding this comment.
I see. I will still need an interface internally to allow the switching between ENO and NO for experimental usage, but can hide that.
Re the instrumentation do we want a separate method for executable normalized operation and normalized document creation, or should I just create a result container that can hold either types?
| * to multiple object types. | ||
| */ | ||
| @PublicApi | ||
| public interface ParsedNormalizedOperation { |
There was a problem hiding this comment.
name is hard - but again I dont think this is Parsed - but I am not sure on a name - but I cant help shaking the feeling its not parsed
There was a problem hiding this comment.
Tracking the naming discussion here: https://github.com/graphql-java/graphql-java/pull/4045/files#r2196386356
There was a problem hiding this comment.
name storming
NormalizedOperation + ExecutableNormalizedOperation
What is the common name here?
- NormalOperation
- GraphQlOperation
- GraphQlNormalizedOperation
timward60
force-pushed
the
timward-instrument-eno
branch
from
9da637b to
61400e6
Compare
I applied one of the suggested names. Let me know if folks prefer something else. |
… NormalizedOperations
|
@bbakerman, @andimarek: I have updated the PR to make the new instrumentation specific, as well as annotating it with |
|
@timward60 just to make you aware: we are adding a profiler: #3976 This will be merged very soon. Maybe if the only goal of the instrumentation is to measure peformance we should consider using Profiler. What do you think? |
I see, let me take a look at that proposal/implementation and get back to you. In general we use the instrumentation callbacks for profiling in many places e.g. logs, open telemetry tracing, metrics (we have various internal systems for different diagnostic aspects). I would need to see if it would still align and that we could still expose it those various consumers. If there is some reluctance in the meantime, I will hold off on this PR until I have time to get back to you on if I foresee any issues with the profiler proposal from our usage above. In the meantime we will continue to use our internal fork to collect some data. Main reason behind this is to measure the move from |
Our usage of GraphQL Java depengs on selection sets which currently backed by executable normalized operations. This PR adds instrumentation that can be used to measure the computation of this component. This is the part 1/3 to support caching of executable normalized operations.