9

So I have a list of objects which I want part or whole to be processed, and I would want to log those objects that were processed.

consider a fictional example:

List<ClassInSchool> classes;
classes
.stream()
.filter(verifyClassInSixthGrade())
.filter(classHasNoClassRoom())
.peek(classInSchool -> log.debug("Processing classroom {} in sixth grade without classroom.", classInSchool)
.forEach(findMatchingClassRoomIfAvailable());

Would using .peek() in this instance be regarded as unintended use of the API?

To further explain, in this question the key takeaway is: "Don't use the API in an unintended way, even if it accomplishes your immediate goal." My question is whether or not every use of peek, short from debugging your stream until you have verified the whole chain works as designed and removed the .peek() again, is unintended use. So if using it as a means to log every object actually processed by the stream is considered unintended use.

Jonas
  • 173
  • 1
  • 3
  • 7
  • 1
    No, the selected answer there is unclear in what is considered unintented use of the API based on the API docs, which is my exact question. Is logging considered debugging in this instance, or is the use of peek actually limited to temporary code while building the actual stream chain. – Jonas Jan 28 '19 at 07:33

4 Answers4

4

The documentation of peek describes the intent as

This method exists mainly to support debugging, where you want to see the elements as they flow past a certain point in a pipeline.

An expression of the form .peek(classInSchool -> log.debug("Processing classroom {} in sixth grade without classroom.", classInSchool) fulfills this intend, as it is about reporting the processing of an element. It doesn’t matter whether you use the logging framework or just print statements, as in the documentation’s example, .peek(e -> System.out.println("Filtered value: " + e)). In either case, the intent matters, not the technical approach. If someone used peek with the intent to print all elements, it would be wrong, even if it used the same technical approach as the documentation’s example (System.out.println).

The documentation doesn’t mandate that you have to distinguish between production environment or debugging environment, to remove the peek usage for the former. Actually, your use would even fulfill that, as the logging framework allows you to mute that action via the configurable logging level.

I would still suggest to keep in mind that for some pipelines, inserting a peek operation may insert more overhead than the actual operation (or hinder the JVM’s loop optimizations to such degree). But if you do not experience performance problems, you may follow the old advice to not try to optimize unless you have a real reason…

Holger
  • 285,553
  • 42
  • 434
  • 765
  • 1
    *as I only want the object processed by the whole chain logged* - the OP comment under one of the answers; in such a case relying on `peek` would be wrong – Eugene Jan 28 '19 at 10:26
  • @Eugene I have the feeling, you’re over-interpreting that comment. To me, it looks like the OP is trying to say that he’s fine with logging only those elements that were actually processed. – Holger Jan 28 '19 at 10:31
  • may be. I mean *by the whole chain* to me is all that hit the terminal operation, using `peek` could log more than that, right? – Eugene Jan 28 '19 at 10:34
  • @Eugene there’s no guaranty that the final consumer’s code ran to completion, but the grammar of the logging message suggests that it is intended to log when it started. Since all elements that passed this point are intended to be passed to the next processing state, i.e. the final consumer, that may be the right/intended semantic, even in the case of unexpected completion, especially in the case of `forEach`, where only an exception could provoke that. – Holger Jan 28 '19 at 10:40
  • ah! good point, you took the code from the question a lot closer than me. I only thought of that to be some example, not something real that the OP is dealing with. – Eugene Jan 28 '19 at 10:44
3

Peek should be avoided as for certain terminal operations it may not be called, see this answer. In that example it would probably be better to do the logging inside the action of forEach rather than using peek. Debugging in this situation means temporary code used for fixing a bug or diagnosing an issue.

tterrag
  • 330
  • 2
  • 10
  • @tterag if I understand the workings of terminal operations correctly it only does not call .peek() in case of the .forEach() would not be called for that object in the stream which actually makes more sense. In that case I would not be logging objects that are not processed. – Jonas Jan 28 '19 at 06:08
  • The trouble is relying on how the terminal operation works under the hood. This behavior can change at any time as it is not defined. Logging objects as they come through the terminal operator is the most reliable way to do what your example is trying to do. – tterrag Jan 28 '19 at 06:14
2

In java streams using .peek() is regarded as to be used for debugging purposes only, would logging be considered as debugging?

It depends on whether your logging code is going to be a permanent fixture of your code, or not.

Only you can really know the real purpose of your logging ...

Also note that the javadoc says:

In cases where the stream implementation is able to optimize away the production of some or all the elements (such as with short-circuiting operations like findFirst, or in the example described in count()), the action will not be invoked for those elements.

So, you are liable to find that in some circumstances peek won't be a reliable way to log (or debug) your pipeline.

In general, adding peek is liable to change the behavior of the pipeline and / or the JVM's ability to optimize it ... in a current or future generation JVM.

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
  • In my opinion the short-circuiting operations do not set my loggin short, as I only want the object processed by the whole chain logged. I have no interest in 400 loglines that actually obscure what happened, since there might only have 40 processed by the termination operation. If that were the reason not to use .peek(), by that logic you should not use .map() either because you might not use a termination operation and that way .map() would not process any objects... – Jonas Jan 28 '19 at 09:09
  • 1
    @Jonas if you want *by the whole chain* - do it in the terminal operation. If you can't, do it afterwards on the result of the terminal operation. `peek` is only to help you understand your stream pipeline and to set it up correctly, nothing more. – Eugene Jan 28 '19 at 09:25
  • 1
    @Jonas - You are free to ignore the advice you are getting from various people. (But if you are going to do that ... why ask for advice?) – Stephen C Jan 28 '19 at 09:55
0

Eh, it's somewhat open to interpretation. Intent is something that's not always easy to determine.

I think the API note was mostly added to discourage an overzealous usage of peek when almost all desirable behaviour can be accomplished without it. It was just too useful for the developers to exclude it completely but they wanted to be clear that its inclusion was not to be taken as an unqualified endorsement; they saw the potential for misuse and they tried to address it.

I suspect - though I'm only speculating - that there were mixed opinions on whether to include it at all, and that including a version with a caveat in the JavaDoc was the compromise.

With that in mind, I think my suggestion for deciding when to use peek would simply be: don't use it unless you have a very good reason to.

In your case, you definitely don't have a good reason to. You're iterating over everything and passing the result to the method findMatchingClassRoomIfAvailable (well, presumably - your example wasn't very good). If you want to log something for each item in the stream then just log it at the top that method.

Is it misuse? I don't think so. Would I write it this way? No.

Michael
  • 41,989
  • 11
  • 82
  • 128