Closed VladoLavor closed 6 months ago
I have tested this locally repeatedly (-count=10000
) with race checker enabled (-race
) and it passed OK, so I assume this will resolve the issue with intermittent unit test failures.
However I have a few comments:
Tracer
usage makes me a bit uneasy, it is referenced from the trace
field of Connection
, which is handled without any lock and could potentially run into race issues when the trace records are being created.Tracer
does seem a bit limiting. For example, what about the overwriting of records? It might be more important for the user to have the last N trace records when running into some issue. Otherwise it is needed to continually call Clear
or re-create the tracer, while user cannot reliably know when the records storage is full. Tracer
always stores all of the requests occurring on the connection, which can easily pollute with records uninteresting to user (e.g. keepalive pings, other non-important channels, ..), since the filtering of records for a specific channel happens when user retrieves the records and not when they are being storedRecord
is missing some info about the requests/replies which might be important to the user, to be specific:
@sknat could you take a look and do a quick review as well?
@ondrej-fabry here are some of my thoughts:
Following up on the discussion we had last week - sorry for the really long review cycle -
and after having played with it quite a bit I guess my position so far align:
1) I think I can also see edge cases happening where you would register a tracer with API messages in flight i.e. receive a message for which we didn't Add()
the WaitGroup.
2) Also aligned here, although it is still unclear to me whether we should aim at having a tracer implementation with bells and whistles in this repository or if we should just export a channel and let users implement their own traces.
3) Filtering might not be required in all cases, although I am under the impression the current implementation is rather specific (i.e. asserting a certain number of messages in advance).
That said I think we can move ahead with this patch as is, and evolve it when adding other usecases.
The way how the API trace is initialized was changed - the trace is no longer initialized (and immediately disabled) by the connection. Instead, the user creates a new trace object and bounds it to a connection.
API changes:
Trace()
method.Enable(bool)
was removed from theTrace
API. Instead, the methodNewTrace(connection, size)
is added.Close()
to release resources used by the tracer.Workflow changes:
vppClient.SendMsg
and the succeeded/failed status of the message is recordedGetRecords
waits until all incoming messages are processed before returning the listSigned-off-by: Vladimir Lavor vlavor@cisco.com