Repository navigation
Conversation
node_values rows are [function_id, parent_node_id, self_value, total_value]. parent_node_id indexes the row order of node_values while function_id indexes the functions list, and the mermaid builder used parent_node_id to index functions directly. The two only coincide while every function owns exactly one node: as soon as one location shows up in two stacks the parent resolves to an unrelated function, so the graph gets an edge that does not exist. It never panics, because a location id cannot exceed the node index, so the mistake stays inside the bounds check and silently claims the wrong caller. Map node ids to function names from the row order once, and use that table for the node's own function and its parent. The same function also passed the translated legend template as an argument to an unrelated format string, so the counts never reached the output and Sprintf appended an %!(EXTRA ...) marker inside the mermaid block. vet rejects that call as soon as the package has a test, so it is corrected together with the tests that expose both problems.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #10176.
Problem
generateFunctionMermaidDiagraminserver/mcp/handle/profile.gobuilds the function-level call graph out ofprofileData.NodeValues. Those rows are[function_id, parent_node_id, self_value, total_value]— seeserver/querier/profile/service/profile.go:parent_node_idindexes the node list (i.e. the row order), whilefunction_idindexesFunctionNames. The builder used the parent node id to index functions:The two index spaces only coincide while every function owns exactly one node. A location that shows up in two stacks gets two nodes, so node indexes run ahead of location ids and every later parent resolves to a different function.
It never panics, which is why this survived: a location id is assigned before its node is appended, so
locationID <= nodeIndexalways holds. The wrong lookup therefore always stays inside the bounds check — the graph just gains an edge that does not exist instead of failing loudly.Concrete case (the shape of the new test):
rootFnalphaFnalphaFngammaFnnode 3is a child ofnode 2, which isalphaFn. The old code looks upFunctionNames[2]=betaFnand emitsbetaFn --> gammaFn.Fix
Build a
node id -> function nametable from the row order once, then use it for the node's own function and for its parent.function_idkeeps indexingFunctionNamesas before; only the parent lookup changes.A second defect in the same function
The legend line passed the translated template as an argument to an unrelated format string:
The counts never reach the output and
Sprintfwritesinfo["显示了 %d 个函数,%d 条调用关系"]%!(EXTRA int=2, int=1)into the mermaid block, which breaks the diagram.go testruns vet, which rejects this call (fmt.Sprintf call needs 1 arg but has 3 args) as soon as the package contains a test file, so it has to be corrected for the tests below to run at all. Both locales keep their%dplaceholders, so the translated string is the format.Verification
Three tests in
server/mcp/handle/profile_test.go:TestDiagramResolvesParentThroughNodeIndex— the table above; assertsalphaFn->gammaFnexists andbetaFn->gammaFndoes not.TestDiagramKeepsLinearStackEdges— a linear stack where node index and function id still coincide; guards the working shape against regressions.TestDiagramLegendCarriesCounts— asserts the legend carries the counts and that no%!marker leaks into the block.Counter-proof: putting
main's version ofprofile.goback and keeping the tests producesso the tests catch exactly these two defects and not something incidental. With the fix:
ok ... 3 passed,gofmt -lclean,go vetclean.Evidence boundary
The tests run against the real
profile.go, the realmodelandcommonpackages, and the realmcp-go/go-simplejsonmodules.server/mcp/config,server/libs/loggerandcommon.CURLPerformwere replaced by minimal stand-ins, because this checkout has no vendored tree and the network here cannot fetch the full module:configonly needs theMConfigpointer to stay nil,loggeronly needs the three methodsprofile.gocalls, andCURLPerformis never reached by these tests. That means the package itself compiles and the tests pass against the file being changed, butmakefor the wholeservermodule was not executed locally — CI'sverify serverwill confirm it.Not covered: no end-to-end MCP
profiletool call against a live querier, and no visual check of the rendered diagram. The fix only changes which function each parent resolves to; the mermaid emission itself is untouched.