Support reading notes from services

[davidkopp]

If merged, resolves #589

[davidkopp]

The current proposal removes the argument -t from the docker logs command. So no timestamps are added as prefixes to the logs. This is required so that the regex is able to find the notes.
An alternative approach would be to still add the timestamps but to change the regex:

def parse_note(self, line):
    if match := fullmatch(r'^(\d{16}) (.+)', line):
        return int(match[1]), match[2]
    return None

https://github.com/green-coding-berlin/green-metrics-tool/blob/main/lib/notes.py#L28

[ribalba]

Great PR again. Minor thing but otherwise looks great

[ArneTR]

I like the issue you raised in #589 and do agree that SCI should readable from any output stream that the GMT orchestrates.

The issue I see with this implementation is that it does not distinguish at which phase the SCI values occur. In order to do that the values must also be sent to the correct phase.

This is a limitation that I opted for when implementing it to have a proof of concept. See also https://docs.green-coding.berlin/docs/measuring/sci/

When we want to now broaden this the SCI must be per phase as if we just collect the output stream from the service it can also generate values in the boot phase already and we have no means of discerning that from the runtime phase.

I have this task still in the backlog but am happy, if you are willing to take this on, to provide some guidance on the possible implementation.
The way it is currently is that we just have a carry that will later be attributed to a fixed phase. The time component has to be integrated in this carry too and in an optimal case the SCI should be a separate reporter.
To do that we have to think if the output streams of the containers can be redirected to a separate memory stream that we can attach to.

Please let me know how you feel about this task, then I am happy to draft a more detailed specification that can be discussed and also changed.

[davidkopp]

This PR is only about supporting the reading of notes from all services, so they are shown in the graphs in the UI. This PR does not change anything about the reading of the SCI score. The reading of the SCI score from the logs of services is already part of the current GMT version.

@ArneTR I think your request about the separation of the reading of the SCI score by phase should be handled with a different issue & PR.

[ArneTR]

I do get the approach for the display in the charts. But wouldn't you find it heavily confusing if the values would show up in the chart but then are not in any form accounted for in the calculations?

I would argue that this is very clear to you, but a confusing feature to bring into for other users. When I see a value in the notes of the chart and also a metric that is called SCI I would expect to have this somehow be correlated / reflected. What do you say?

Happy to discuss this further, because maybe I am misunderstanding the extent of the PR at the moment.

[davidkopp]

The reason for me to create this PR was to be able to see when async operations are ending by looking at the notes in the charts. In some of my measurement runs the async operations were ending after the end of the runtime phase. To improve that, the display of the notes from the service in the charts would be quite helpful.

I understand that the calculation of the SCI metric can be quite confusing at the moment because of the fixed phase. However, I don't understand why you want to fix this in this PR. This PR doesn't change anything about the reading of the SCI ticks.

Reading of the SCI ticks is already part of GMT (https://github.com/green-coding-berlin/green-metrics-tool/blob/main/runner.py#L1147-L1150):

if container_info['read-sci-stdout']:
    for line in log.stdout.splitlines():
        if match := re.findall(r'GMT_SCI_R=(\d+)', line):
            self._sci['R'] += int(match[0])

[ArneTR]

Thanks for clarifying that. I had a hunch that I was misunderstanding something.

I get the call for the logging option for container logs in general especially when you cannot produce the logs through the async processes.

The problematic regarding the SCI got onto my radar because I saw that you touched these lines in the PR. Now looking more closely the issue that I was talking about is actually already present and I misremembered the implementation.

So this PR is fine and happy to take it in. Will open a separate issue for the SCI fixing.

[ArneTR]

The check for the Github pipeline is failing but I validated locally. Tests run fine.

ty david!