feat: Add support for MeterReportService #231

jiang1997 · 2022-08-22T16:30:59Z

Add support for MeterReportService

Behavior

This service runs along with the agent like other services and reports roughly every 20 seconds.

Syntactic sugar

Counter

c = Counter('c2', CounterMode.INCREMENT)
c.build()

# increase Counter c by the time the with-wrapped codes consumed
with c.create_timer():
    # some codes may consume a certain time

c = Counter('c3', CounterMode.INCREMENT)
c.build()

# increase Counter c by num once counter_decorator_test gets called
@Counter.increase(name='c3', num=2)
def counter_decorator_test():
    # some codes

c = Counter('c4', CounterMode.INCREMENT)
c.build()

# increase Counter c by the time counter_decorator_test consumed
@Counter.timer(name='c4')
def counter_decorator_test(s):
    # some codes may consume a certain time

Histogram

h = Histogram('h3', [i / 10 for i in range(10)])
h.build()

# Histogram h will record the time the with-wrapped codes consumed
with h.create_timer():
    # some codes may consume a certain time

h = Histogram('h2', [i / 10 for i in range(10)])
h.build()

# Histogram h will record the time histogram_decorator_test consumed
@Histogram.timer(name='h2')
def histogram_decorator_test(s):
    time.sleep(s)

Simple test

insert following codes below this line agent/__init__.py#L151

from skywalking.meter.gauge import Gauge
import gc

def generator_for_gc_g0():
    while(True):
        yield gc.get_stats()[0]['collected']

def generator_for_gc_g1():
    while(True):
        yield gc.get_stats()[1]['collected']

def generator_for_gc_g2():
    while(True):
        yield gc.get_stats()[2]['collected']


gc_g0_meter = Gauge("gc_g0", generator_for_gc_g0())
gc_g0_meter.build()

gc_g1_meter = Gauge("gc_g1", generator_for_gc_g1())
gc_g1_meter.build()

gc_g2_meter = Gauge("gc_g2", generator_for_gc_g2())
gc_g2_meter.build()

Superskyyy · 2022-08-22T16:41:54Z

Please try to run the linter with make lint or make dev-check(if you don't want venv egg. to be purged) command. It will be quicker to fix things.

sonatype-lift · 2022-08-22T17:25:52Z

skywalking/meter/counter.py

+        super().__init__(name, tags)
+        self.count = 0
+        self.previous = 0
+        self.mode = mode


Incompatible attribute type: Attribute mode declared in class Counter has type BoundMethod[typing.Callable(Counter.mode)[[Named(self, Counter), Named(mode, unknown)], typing.Any], Counter] but is used as type CounterMode.

Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Superskyyy · 2022-08-22T17:48:55Z

skywalking/client/__init__.py

@@ -28,6 +28,9 @@ class TraceSegmentReportService(object):
    def report(self, generator):
        raise NotImplementedError()

+class MeterReportService(object):
+    def report(self, generator):


also report_batch?

Yes, it could be used. These two report receive different generators. The former needs a generator that generates MeterData and the latter MeterDataCollection.

@jiang1997 I collected the call times of an interface through Counter and reported it to skywalking. Now I want to display this metric on the ui page. How do I do it? Thank you!

Learn MAL and Dashboard setup.

https://skywalking.apache.org/docs/main/next/en/setup/backend/backend-meter/

https://skywalking.apache.org/docs/main/next/en/concepts-and-designs/mal/

Examples, https://github.com/apache/skywalking/tree/89d14e6256338840cd4a9b37f76980c8558e50c3/oap-server/server-starter/src/main/resources/meter-analyzer-config/

Dashboard, https://skywalking.apache.org/docs/main/next/en/ui/readme/

@March225
This is just a simple supplement to wu-sheng's advice for the sake of simplicity.

Try to append your metric's definition to the server-side config file like these predefined metrics as below
https://github.com/apache/skywalking/blob/e2f1c0eadb52082e16f3abfc29dd8860000b596b/oap-server/server-starter/src/main/resources/meter-analyzer-config/python-runtime.yaml#L31-L36

Then edit the dashboard and add a new widget to show your metric.
If nothing goes wrong, you should find your metric here.

skywalking/meter/histogram.py

skywalking/meter/meter.py

sonatype-lift · 2022-08-23T11:39:59Z

skywalking/meter/histogram.py

@@ -0,0 +1,120 @@
+#


Parsing failure: Could not parse file at skywalking/meter/histogram.py

Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.

When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

jiang1997 · 2022-08-23T14:51:38Z

Sorry for messing up the commit history.

jiang1997 · 2022-08-23T15:10:38Z

skywalking/agent/__init__.py

@@ -37,7 +38,7 @@
 __started = False
 __protocol = Protocol()  # type: Protocol
 __heartbeat_thread = __report_thread = __log_report_thread = __query_profile_thread = __command_dispatch_thread \
-    = __send_profile_thread = __queue = __log_queue = __snapshot_queue = __finished = None
+    = __send_profile_thread = meter_service_thread = __queue = __log_queue = __snapshot_queue = __finished = None


I tried to name meter_service_thread as __meter_service_thread to comply with the naming convention here. But only to find that double underscore has its own specific behaviour when importing double underscore variable from another file. More details here. https://stackoverflow.com/questions/1301346/what-is-the-meaning-of-single-and-double-underscore-before-an-object-name

@jiang1997 Thanks, good finding! I believe __ prefixed variables are not supposed to be referred to by external classes, but when it is defined outside of classes, it doesn't do much other than the meaning of "you probably shouldn't use (import) this".

Anyway, I see the current implementation bit out of sync with the previous reporter styles.

That is, you provided a new MeterService(Thread), and the result of defining threads outside of agent/__init__.py is that the code base becomes weird in terms of calling topology, and there are some odd references because of it.

Maybe try two steps to make our agent startup behaviour better organized, given __init__.py was never intended to be this big and also not supposed to have logic scattered around in other modules.

Follow the existing style to see how you can write the MeterService class without the Threading part, like the existing profiling service (put the threading part back to agent/__init__.py. (If you understand this already, look at step 2)

Try to make the existing __init__.py into more organized modules at the agent/ directory; essentially, minimize agent/__init__.py and decouple the current mess. (If you meet obstacles, revert to step 1, and it will be good enough)

Superskyyy · 2022-08-23T15:39:09Z

Sorry for messing up the commit history.

It's okay, everything will be squashed in the end.

jiang1997 · 2022-08-23T16:01:45Z

Sorry for messing up the commit history.

It's okay, everything will be squashed in the end.

Yes, it makes sense for both the end of this pr and the universe.

wu-sheng · 2022-08-24T00:09:06Z

Sorry for messing up the commit history.

It's okay, everything will be squashed in the end.

Let's clear this policy. The result is YES, we would squash all the commits to merge as one single commit in the main branch.
But this is only a tradeoff, as many new contributors don't know how to make sure the commit logs clear and readable.

A real good commit log should be like this

apache/skywalking@8a80700

Introducing the clear context of why this PR changed in this way and how we apply the changes. Multiple commit logs could be automatically merged into one commit log in merging.

Superskyyy · 2022-08-24T05:17:21Z

A real good commit log should be like this

apache/skywalking@8a80700

Introducing the clear context of why this PR changed in this way and how we apply the changes. Multiple commit logs could be automatically merged into one commit log in merging.

Really good example, thanks!

jiang1997 · 2022-08-24T05:25:59Z

Sorry for messing up the commit history.

It's okay, everything will be squashed in the end.

Let's clear this policy. The result is YES, we would squash all the commits to merge as one single commit in the main branch. But this is only a tradeoff, as many new contributors don't know how to make sure the commit logs clear and readable.

A real good commit log should be like this

apache/skywalking@8a80700

Introducing the clear context of why this PR changed in this way and how we apply the changes. Multiple commit logs could be automatically merged into one commit log in merging.

Thanks for elaborating!

jiang1997 · 2022-08-25T17:38:33Z

It's kind of weird. I passed make lint in my local environment but still failed in Actions.
Here is the message from CI / License and Lint (pull_request)

venv/bin/flake8 .
./tests/plugin/base.py:35:1: B024 TestPluginBase is an abstract base class, but it has no abstract methods. Remember to use @abstractmethod, @abstractclassmethod and/or @abstractproperty decorators.
class TestPluginBase(ABC):
^
./skywalking/agent/protocol/__init__.py:22:1: B024 Protocol is an abstract base class, but it has no abstract methods. Remember to use @abstractmethod, @abstractclassmethod and/or @abstractproperty decorators.
class Protocol(ABC):
^
./skywalking/trace/span.py:36:1: B024 Span is an abstract base class, but it has no abstract methods. Remember to use @abstractmethod, @abstractclassmethod and/or @abstractproperty decorators.
class Span(ABC):
^
3     B024 Protocol is an abstract base class, but it has no abstract methods. Remember to use @abstractmethod, @abstractclassmethod and/or @abstractproperty decorators.
3
make: *** [Makefile:46: lint] Error 1
Error: Process completed with exit code 2.

Superskyyy · 2022-08-25T18:05:50Z

It's kind of weird. I passed make lint in my local environment but still failed in Actions. Here is the message from CI / License and Lint (pull_request)

Indeed weird, but what the linter pointed out was right. I'm not sure what happened.

jiang1997 · 2022-08-26T15:54:56Z

It's kind of weird. I passed make lint in my local environment but still failed in Actions. Here is the message from CI / License and Lint (pull_request)

Indeed weird, but what the linter pointed out was right. I'm not sure what happened.

How about creating a new pr to fix these errors that linter reported? I'm willing to.

Superskyyy · 2022-08-26T16:47:19Z

It's kind of weird. I passed make lint in my local environment but still failed in Actions. Here is the message from CI / License and Lint (pull_request)

Indeed weird, but what the linter pointed out was right. I'm not sure what happened.

How about creating a new pr to fix these errors that linter reported? I'm willing to.

Well technically you could, but the PR cannot be allowed to merge until CI passes, unless you can bypass it.

kezhenxu94 · 2022-08-27T05:39:52Z

@jiang1997 can you please fix the linter errors in another PR. This should be because we didn’t pin the linter version and the linter updated recently. I just tried rerun a previously successful CI and it now failed. Anyway, the linter errors in this PR look reasonable to be fixed. Please fix them in another PR

Superskyyy · 2022-09-07T18:46:09Z

Generally looks good; please add the PVM metrics in the following PR and provide additional E2E test cases (refer to the currently commented TODO in the e2e .yaml file)

@kezhenxu94 Please also take a look :)

jiang1997 · 2022-09-07T18:55:31Z

Generally looks good; please add the PVM metrics in the following PR and provide additional E2E test cases (refer to the currently commented TODO in the e2e .yaml file)

@kezhenxu94 Please also take a look :)

Got it.

wu-sheng · 2022-09-08T13:21:30Z

I think you should update the docs about meter APIs. @Superskyyy We also miss the tracing APIs doc.

Superskyyy · 2022-09-08T15:00:52Z

I think you should update the docs about meter APIs. @Superskyyy We also miss the tracing APIs doc.

Ok, we will provide both API docs next!

wu-sheng · 2022-09-09T04:21:42Z

reports roughly every one second.

I noticed this, is it configurable? Usually metrics don't report so frequently considering backend load and necessarily.
10-20s period is a better default value

jiang1997 · 2022-09-09T04:45:08Z

reports roughly every one second.

I noticed this, is it configurable? Usually metrics don't report so frequently considering backend load and necessarily. 10-20s period is a better default value

Thanks for the suggestion. I will make it configurable later.

Superskyyy · 2022-09-10T04:20:49Z

Thanks! Looking good.

kezhenxu94

Hi @jiang1997, thanks for the patience and hard work!! I'm going to merge this PR to unblock your next steps, also I left some comments that you might want address in a following minor PR, or in your next large PR with the PVM metrics.

kezhenxu94 · 2022-09-11T08:26:50Z

docs/en/setup/advanced/MeterReporter.md

@@ -0,0 +1,110 @@
+# Python Agent Meter Reporter


Can you please also add a link in the menu to this doc?

skywalking-python/docs/menu.yml

Lines 34 to 38 in aad9034

catalog:

- name: "Log Reporter"

path: "/en/setup/advanced/LogReporter"

- name: "Manual Instrumentation"

path: "/en/setup/advanced/API"

kezhenxu94 · 2022-09-11T08:28:41Z

docs/en/setup/advanced/MeterReporter.md

+## Counter
+* `Counter` API represents a single monotonically increasing counter, automatic collect data and report to backend.
+```python
+tg_ls = [MeterTag("key", "value")]


Would be awesome if we can just pass in a Python tuple

Suggested change

tg_ls = [MeterTag("key", "value")]

tg_ls = [("key", "value")]

Sounds like a good idea.

kezhenxu94 · 2022-09-11T08:31:58Z

docs/en/setup/advanced/MeterReporter.md

+### Syntactic sugars
+```python
+c = Counter('c2', CounterMode.INCREMENT)
+c.build()


Looks like every meter should invoke build after construction, what about just embed the build inside the construction of the meters? So that users won't forget to call the build method?

You are right. This is the sequelae of imitating java agent.

The reason we need build method because we separate tags definition from construction.

The reason we need build method because we separate tags definition from construction.

Thanks for reminding. Here also separate tags definition from construction, so seems build is still needed.

feat: meter service

ff10f43

Superskyyy requested review from Superskyyy and kezhenxu94 August 22, 2022 16:32

jiang1997 changed the title ~~feat: meter service~~ feat: Add support for MeterReportService Aug 22, 2022

Superskyyy added feature New feature core labels Aug 22, 2022

Superskyyy added this to the 1.0.0 milestone Aug 22, 2022

Merge branch 'master' into feat_meter

bb75462

sonatype-lift bot reviewed Aug 22, 2022

View reviewed changes

Superskyyy reviewed Aug 22, 2022

View reviewed changes

skywalking/meter/histogram.py Outdated Show resolved Hide resolved

Superskyyy reviewed Aug 22, 2022

View reviewed changes

skywalking/meter/histogram.py Outdated Show resolved Hide resolved

Superskyyy reviewed Aug 22, 2022

View reviewed changes

skywalking/meter/meter.py Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Aug 23, 2022

View reviewed changes

skywalking/meter/meter.py Outdated Show resolved Hide resolved

jiang1997 force-pushed the feat_meter branch 2 times, most recently from 99a357d to 5c05dcd Compare August 23, 2022 11:31

sonatype-lift bot reviewed Aug 23, 2022

View reviewed changes

jiang1997 force-pushed the feat_meter branch from 5c05dcd to eadaf12 Compare August 23, 2022 11:48

style: formatting

386f626

jiang1997 force-pushed the feat_meter branch from eadaf12 to 386f626 Compare August 23, 2022 12:08

fix: add __init__.py for meter

605a853

jiang1997 commented Aug 23, 2022

View reviewed changes

refactor: refactor MeterService class following 'profile'

3680eb9

Superskyyy and others added 2 commits September 2, 2022 02:00

Merge branch 'master' into feat_meter

a926b3b

feat: add decorator 'timer' for Counter

3b6299b

jiang1997 marked this pull request as ready for review September 8, 2022 04:50

Superskyyy self-requested a review September 9, 2022 14:42

Superskyyy assigned jiang1997 Sep 9, 2022

doc: add a doc for MeterReporter

9fe266d

jiang1997 force-pushed the feat_meter branch from 321ce2d to 9b77a18 Compare September 9, 2022 19:06

feat: make report peroid configurable

4847c40

jiang1997 force-pushed the feat_meter branch from 9b77a18 to 4847c40 Compare September 9, 2022 19:13

Merge branch 'master' into feat_meter

4cc75d9

Merge branch 'master' into feat_meter

aad9034

kezhenxu94 approved these changes Sep 11, 2022

View reviewed changes

kezhenxu94 merged commit 473ece7 into apache:master Sep 11, 2022

kezhenxu94 mentioned this pull request Oct 4, 2022

doc: new ability: metrics #240

Merged

	catalog:
	- name: "Log Reporter"
	path: "/en/setup/advanced/LogReporter"
	- name: "Manual Instrumentation"
	path: "/en/setup/advanced/API"

	tg_ls = [MeterTag("key", "value")]
	tg_ls = [("key", "value")]

feat: Add support for MeterReportService #231

feat: Add support for MeterReportService #231

Conversation

jiang1997 commented Aug 22, 2022 • edited Loading

Behavior

Syntactic sugar

Counter

Histogram

Simple test

Superskyyy commented Aug 22, 2022

sonatype-lift bot Aug 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiang1997 Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

sonatype-lift bot Aug 23, 2022

Choose a reason for hiding this comment

jiang1997 commented Aug 23, 2022

jiang1997 Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Superskyyy Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Superskyyy commented Aug 23, 2022

jiang1997 commented Aug 23, 2022

wu-sheng commented Aug 24, 2022

Superskyyy commented Aug 24, 2022

jiang1997 commented Aug 24, 2022

jiang1997 commented Aug 25, 2022

Superskyyy commented Aug 25, 2022

jiang1997 commented Aug 26, 2022

Superskyyy commented Aug 26, 2022

kezhenxu94 commented Aug 27, 2022

Superskyyy commented Sep 7, 2022

jiang1997 commented Sep 7, 2022

wu-sheng commented Sep 8, 2022

Superskyyy commented Sep 8, 2022

wu-sheng commented Sep 9, 2022

jiang1997 commented Sep 9, 2022

Superskyyy commented Sep 10, 2022 • edited Loading

kezhenxu94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiang1997 commented Aug 22, 2022 •

edited

Loading

jiang1997 Mar 3, 2023 •

edited

Loading

jiang1997 Aug 23, 2022 •

edited

Loading

Superskyyy Aug 23, 2022 •

edited

Loading

Superskyyy commented Sep 10, 2022 •

edited

Loading