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

chore(rfc): Extend core module system to support metrics #1130

Open
wants to merge 1 commit into
base: dustin/eng-5535-create-an-rfc
Choose a base branch
from

Conversation

kaialang
Copy link

@kaialang kaialang commented Aug 30, 2024

Motivation and Context

It extends the open core module system RFC to enable custom metric modules.

  • Expose both the entering point and existing point. RouterRequestHook, SubgraphRequestHook and OperationParse/Normalize/PlanHook It enables taking end to end measurements.
  • Create grouped interfaces: RouterHook, SubgraphHook and OperationHook for easier integration
  • For response hooks, a correct/meaningful statusCode should be given for easier processing.
  • For Error hooks , input error is an object of core.errorType, so the error can be easily casted.
  • Operation name/type should be always included in inputs.
  • Removed Telemetry Hooks, as with the above updates and refactor telemetry to be part of the the module system, it can be achieved by implementing the default metrics module.

New findings need to be discussed:

  • SchemaUsage info needs to populated despite of graphqlMetricsConfig value, graphqlMetricConfig should only turn on/off for the export to control plane part.

Added one example use case.

TODO

@kaialang kaialang force-pushed the extend_core_module_for_metrics branch 2 times, most recently from eda719a to 0efb9d1 Compare August 30, 2024 03:41
@StarpTech StarpTech changed the title Extend core module system to support metrics chore(rfc): Extend core module system to support metrics Sep 8, 2024
@kaialang kaialang force-pushed the extend_core_module_for_metrics branch from 0efb9d1 to d091eab Compare September 8, 2024 23:18
@kaialang kaialang force-pushed the extend_core_module_for_metrics branch from d091eab to d083bff Compare September 8, 2024 23:23
@@ -235,21 +251,6 @@ type ApplicationErrorHook interface {
OnAppError(err error)
}

// TelemetryHooks are called when a span is created
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I wasn't a fan of it either. I think we can solve this with generic hooks and let the developer decide what to do at this time.

}

type GraphQLOperationPlanHook interface {
// OnPlan is called when an operation is planned
type GraphQLOperationPostNormalizeHook interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the pre/post capabilities.

return nil
}

func (m *MyModule) OnRouterResponse(res *core.RouterResponse, err error) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

The use case would be covered but I'm still not happy how data is shared across hooks. I think it can become cumbersome, if you have to use the context api. It would be great to have an API to hook into pre/post without the need to share data.

Copy link

github-actions bot commented Oct 3, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 3, 2024
@StarpTech StarpTech removed the Stale label Oct 3, 2024
Copy link

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 18, 2024
@StarpTech StarpTech removed the Stale label Oct 18, 2024
Copy link

github-actions bot commented Nov 2, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants