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

feat: add custom tracing spans with jina>=3.12.0 #861

Merged
merged 6 commits into from
Dec 2, 2022

Conversation

girishc13
Copy link
Contributor

@girishc13 girishc13 commented Nov 16, 2022

Goals:

  • update to jina>=3.12.0
  • update Executors with tracing support:
    • add NoOpTracer if tracing is disabled so that the operations with self.tracer become no-op code. The NoOpTracer is part of the opentelemetry-api library downloaded by jina standard requirements.
    • add new spans to measure various operations in the encode requests method.
    • more customizations/spans if needed.
  • update documentation for Executors.

I will post a link to the completed Blog so that the usage of tracing is clear.

@girishc13
Copy link
Contributor Author

I will fix the failing pr after upgrading the jina version to 3.11.1.

server/setup.py Show resolved Hide resolved
@girishc13 girishc13 changed the title feat: add custom tracing spans with jina>=3.11.0 feat: add custom tracing spans with jina>=3.12.0 Nov 25, 2022
@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #861 (564d25c) into main (67f551c) will increase coverage by 2.96%.
The diff coverage is 95.04%.

@@            Coverage Diff             @@
##             main     #861      +/-   ##
==========================================
+ Coverage   80.28%   83.24%   +2.96%     
==========================================
  Files          22       22              
  Lines        1633     1498     -135     
==========================================
- Hits         1311     1247      -64     
+ Misses        322      251      -71     
Flag Coverage Δ
cas 83.24% <95.04%> (+2.96%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/clip_server/executors/clip_onnx.py 85.22% <94.87%> (+3.28%) ⬆️
server/clip_server/executors/clip_tensorrt.py 94.59% <95.00%> (+1.49%) ⬆️
server/clip_server/executors/clip_torch.py 86.90% <95.23%> (+3.57%) ⬆️
server/clip_server/model/pretrained_models.py 98.41% <0.00%> (ø)
server/clip_server/model/flash_attention.py 22.22% <0.00%> (+2.22%) ⬆️
server/clip_server/model/openclip_model.py 93.10% <0.00%> (+3.44%) ⬆️
server/clip_server/model/model.py 75.09% <0.00%> (+4.98%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions github-actions bot added size/l and removed size/m labels Nov 28, 2022
@girishc13 girishc13 force-pushed the opentelmetry-integration branch from 971c2b2 to 2c546c5 Compare November 28, 2022 12:41
@girishc13 girishc13 marked this pull request as ready for review November 28, 2022 13:45
server/clip_server/executors/clip_torch.py Show resolved Hide resolved
self._pool = ThreadPool(processes=num_worker_preprocess)

self._model = CLIPModel(name, device=self._device, jit=jit, **kwargs)
self._tokenizer = Tokenizer(name)
self._image_transform = clip._transform_ndarray(self._model.image_size)

if not self.tracer:
self.tracer = NoOpTracer()
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the tracker is enabled by default. Is it possible to use it as an optional function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The NoOpTracer is a no operations implementation means that it's a pass through, no overhead implementation that is available in the jina[standard] installation. This is a requirement to have a no-operations tracer otherwise using the context managers like with self.tracer.start_as_current_span will become more complicated. The BaseExecutor already sets the value to None but the NoOpTracer should be used for custom spans even if tracing is turned off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the self.tracer needs to created once based on the configuration arguments otherwise there would be unnecessary overhead trying to create an instance for every request.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added size/m and removed size/l labels Nov 29, 2022
Copy link
Member

@numb3r3 numb3r3 left a comment

Choose a reason for hiding this comment

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

LGTM

@numb3r3 numb3r3 merged commit 1eebdd7 into main Dec 2, 2022
@numb3r3 numb3r3 deleted the opentelmetry-integration branch December 2, 2022 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants